Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add variant of fs.create{Read,Write}Stream to fs/promises #21342

Closed
dead-claudia opened this issue Jun 15, 2018 · 5 comments
Closed

Add variant of fs.create{Read,Write}Stream to fs/promises #21342

dead-claudia opened this issue Jun 15, 2018 · 5 comments

Comments

@dead-claudia
Copy link

dead-claudia commented Jun 15, 2018

Basically, I'd like to see those two common core APIs ported to use promises for the initial ready/error.

// Naïve implementation, but could be very dramatically improved.
// `createWriteStream` is similar.
function createReadStream(...args) {
    return new Promise((resolve, reject) => {
        const stream = fs.createReadStream(...args)

        function ready() {
            stream.removeListener("ready", ready)
            stream.removeListener("error", fail)
            resolve(stream)
        }

        function fail(e) {
            stream.removeListener("ready", ready)
            stream.removeListener("error", fail)
            reject(e)
        }

        stream.on("ready", ready)
        stream.on("error", fail)
    })
}

This amounts to the vast majority of my routine promise event wrappers while using fs/promises normally.

@simonkcleung
Copy link

To clean up the resource, you should use finally, which can also address the problem when you throw an error.

@dead-claudia
Copy link
Author

@simonkcleung This isn't about cleanup. It's about errors during initialization.

@ChALkeR
Copy link
Member

ChALkeR commented Jun 16, 2018

I would prefer this to be a chance to change the readable/writeable streams returned by fs module.

E.g. something like a .promise property on readable and writable streams that resolves on close for writeable and end for readable and rejects on error. #20909 is related.

@dead-claudia
Copy link
Author

@ChALkeR I would be mildly okay with that, too, but I'd still like the ready/error issue also resolved. And yes, #20909 is one of the driving reasons for me filing this bug. (It's the most common case where I need that feature, and I've written a dedicated helper for this probably a few dozen times already.)

Using this + your addition would convert this:

// Old with callbacks
const input = fs.createWriteStream(infile)
const output = fs.createWriteStream(outfile)
let readyIn = false
let readyOut = false

input.on("error", error)
output.on("error", error)

input.on("ready", onReadyIn)
output.on("ready", onReadyOut)

input.on("close", close)
output.on("close", close)
output.on("finish", close)

function onReadyIn() {
    readyIn = true
    if (readyOut) writeData()
}

function onReadyOut() {
    readyOut = true
    if (readyIn) writeData()
}

function error(e) {
    cleanup()
    if (readyIn) input.destroy(e)
    if (readyOut) output.destroy(e)
}

function close() {
    cleanup()
    if (readyIn) input.destroy()
    if (readyOut) output.destroy()
}

function cleanup() {
    input.removeListener("error", error)
    output.removeListener("error", error)
    input.removeListener("ready", onReadyIn)
    output.removeListener("ready", onReadyOut)
}

to this:

// New with promises
let input, output
try {
    input = await fsPromises.createReadStream(infile)
    output = await fsPromises.createReadStream(outfile)
    // write data
} finally {
    if (input != null) { input.destroy(); await input.closePromise }
    if (output != null) { output.destroy(); await output.closePromise }
}

I don't like the name .promise, though - it's too generic for that specific of a thing. It's also not FS-specific. Also, if you're going to do that, you may want to include close for readable, too, since that's what fires when you .destroy a readable stream.

@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

This is already possible given that a stream.Readable can be used as an async generator and a Promises version of fileHandle.write() exists. Closing.

@jasnell jasnell closed this as completed Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants