Move file reading and writing to streaming #164

Zazama · 2023-04-06T21:56:53Z

This PR implements file streaming for reading files using the NodeID3.read, NodeID3.write (and update) function.
By doing so, we support reading/writing big files and go easy on memory.

We read/write the file in chunks of max. 20 megabytes (if you run node.js, you probably have 20mb to spare)
The reader will automatically detect the start of an ID3 Tag and extract it fully into a buffer
- This assumes that ID3 Tags are small enough to fit into memory
- If it were bigger, the reading would fail anyways, because the parsed information would not fit into memory
Code is slightly repeated because of sync/async support. I haven't found a better way yet
On write, we create a temp file next to the original one
- This is done using the "tmp" module because we need collision handling and I think it's good to keep the files in the same folder so it can be found easily on failure.
- First, the new ID3 Tag is written at the beginning
- Then the rest of the old file is streamed into the new one. If an old ID3 Tag is found, it is skipped
The type definition of a NodeID3.write callback should not return a buffer if a file is being manipulated. This is also how it is documented in the README.md

Related issue: #161

pbricout

I did a partial review and addressed most of the points here, this is still work in progress there is bit more to do, but this is a good start.

pbricout · 2023-04-14T22:39:31Z

src/file-write.ts

+            streamOriginalIntoNewFileSync(readFileDescriptor, writeFileDescriptor)
+        })
+    })
+    fs.unlinkSync(filepath)


This is unnecessary, rename will overwrite the destination and is safer as the operation will be atomic.

pbricout · 2023-04-14T23:00:45Z

src/file-write.ts

+function getTmpFilePathSync(filepath: string): string {
+    const parsedPath = path.parse(filepath)
+    return tmp.tmpNameSync({
+        tmpdir: parsedPath.dir,
+        template: `${parsedPath.base}.tmp-XXXXXX`,
+    })
+}
+
+function getTmpFileAsync(filepath: string, callback: tmp.TmpNameCallback) {
+    const parsedPath = path.parse(filepath)
+    tmp.tmpName({
+        tmpdir: parsedPath.dir,
+        template: `${parsedPath.base}.tmp-XXXXXX`,
+    }, (err, filename) => {
+        callback(err, filename)
+    })
+}


The common part could be factored in a function getTmpNameOptions(filename) => then tmp.tmpName(getTmpNameOptions(flename)).

pbricout · 2023-04-14T23:01:16Z

src/file-write.ts

+
+function getTmpFileAsync(filepath: string, callback: tmp.TmpNameCallback) {
+    const parsedPath = path.parse(filepath)
+    tmp.tmpName({


Could we promisify that one too so we can use await and simplify writeId3TagToFileAsync().

pbricout · 2023-04-14T23:02:11Z

src/file-write.ts

+                await streamOriginalIntoNewFileAsync(readFileDescriptor, writeFileDescriptor)
+            })
+        }).then(async () => {
+            await fsUnlinkPromise(filepath)


This is unnecessary, rename will overwrite the destination and is safer as the operation will be atomic.

pbricout · 2023-04-14T23:04:58Z

src/util-file.ts

+export const fsOpenPromise = promisify(fs.open)
+export const fsReadPromise = promisify(fs.read)
+export const fsClosePromise = promisify(fs.close)
+export const fsWritePromise = promisify(fs.write)
+export const fsUnlinkPromise = promisify(fs.unlink)
+export const fsRenamePromise = promisify(fs.rename)


We could export an object:

export const fsPromise = { open: promisify(fs.open) // ... }

pbricout · 2023-04-15T09:01:40Z

src/file-write.ts

+import * as fs from 'fs'
+import { findId3TagPosition, getId3TagSize } from "./id3-tag"
+
+const FileBufferSize = 20 * 1024 * 1024


This should be an option with a default, this will allow to write tests with smaller files.

pbricout · 2023-04-15T09:11:57Z

src/file-write.ts

+
+const FileBufferSize = 20 * 1024 * 1024
+
+export function writeId3TagToFileSync(filepath: string, id3Tag: Buffer) {


The temp file is not deleted in case of error.

pbricout · 2023-04-15T09:12:03Z

src/file-write.ts

+    fs.renameSync(tmpFile, filepath)
+}
+
+export function writeId3TagToFileAsync(filepath: string, id3Tag: Buffer, callback: (err: Error|null) => void) {


The temp file is not deleted in case of error.

pbricout · 2023-04-15T16:03:44Z

src/api/write.ts

 */
 export function write(
-    tags: WriteTags, filebuffer: string | Buffer, callback: WriteCallback
+    tags: WriteTags, filebuffer: string | Buffer, callback: WriteFileCallback | WriteCallback


Let's not make the existing API even more complicated, let's take this opportunity to create a new release with a simplified API.

pbricout · 2023-04-15T16:04:30Z

src/file-write.ts

+
+export function writeId3TagToFileSync(filepath: string, id3Tag: Buffer) {
+    const tmpFile = getTmpFilePathSync(filepath)
+    processFile(filepath, 'r', (readFileDescriptor) => {


This fails if the source file does not exist, but we want to support this case, if there is no source file, we should just write.

pbricout · 2023-04-18T22:35:46Z

src/file-write.ts

+    fs.renameSync(tmpFile, filepath)
+}
+
+export function writeId3TagToFileAsync(filepath: string, id3Tag: Buffer, callback: (err: Error|null) => void) {


Ideally we could optimize by doing the process of the buffer during an async read/write operation, but this is a bit more complicated, let's leave it for another PR.

pbricout · 2023-04-20T22:01:46Z

src/file-write.ts

+    const buffer = Buffer.alloc(FileBufferSize)
+    let data
+    while((data = getNextBufferSubarraySync(readFileDescriptor, buffer)).length) {
+        const id3TagPosition = findId3TagPosition(data)


There is a bug, we may miss the tag if across two readings, we need to use the same trick than in read.

Zazama linked an issue Apr 6, 2023 that may be closed by this pull request

Support large files #161

Open

Zazama removed a link to an issue Apr 6, 2023

Support large files #161

Open

Zazama added 3 commits April 7, 2023 16:07

Stream files on read instead of fully consuming them

775a8f3

Fix fs close bugs and refactor

69f0c10

Add NodeJS 10 support for file streaming

1848658

Zazama force-pushed the filestream branch from a348a7f to 1848658 Compare April 7, 2023 14:09

Add file streaming to write operation

0c93210

Zazama changed the title ~~Move file reading to streaming~~ Move file reading and writing to streaming Apr 7, 2023

Zazama added 2 commits April 7, 2023 18:29

eslint fixes

6160f45

Call callback on success

7a8672d

pbricout reviewed Apr 16, 2023

View reviewed changes

pbricout reviewed Apr 18, 2023

View reviewed changes

pbricout reviewed Apr 20, 2023

View reviewed changes


		const FileBufferSize = 20 * 1024 * 1024

		export function writeId3TagToFileSync(filepath: string, id3Tag: Buffer) {

Move file reading and writing to streaming #164

Are you sure you want to change the base?

Move file reading and writing to streaming #164

Uh oh!

Conversation

Zazama commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pbricout left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Zazama commented Apr 6, 2023 •

edited

Loading