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

fix: Match promisified fs.read call return with original implementation #2128

Merged
merged 4 commits into from
Nov 17, 2020

Conversation

Mike-Dax
Copy link
Contributor

@Mike-Dax Mike-Dax commented Nov 16, 2020

What's the problem this PR addresses?

Fixes #2127
Fixes serialport/node-serialport#2158

...

How did you fix it?

Node stores whether or not the promisified version should have a different return shape in a symbol (internalUtil.customPromisifyArgs) that's private, we can't access that.

https://github.com/nodejs/node/blob/dc79f3f37caf6f25b8efee4623bec31e2c20f595/lib/fs.js#L559-L560

However the util.promisify function caches implementations in a cache key that's a global symbol (available at util.promisify.custom). We can pre-populate that cache with a version of the function that works.

https://github.com/nodejs/node/blob/ba684805b6c0eded76e5cd89ee00328ac7a59365/lib/internal/util.js#L274,L281

This PR implements a pre-cached function that returns an object of the same shape as util.promisify(fs.read) would on a standard node system
...

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test to make sure we don't regress on this in the future?

@arcanis
Copy link
Member

arcanis commented Nov 16, 2020

For that, a unit test in patchedFs.test.ts will do the trick

// and
// https://github.com/nodejs/node/blob/ba684805b6c0eded76e5cd89ee00328ac7a59365/lib/internal/util.js#L293
// @ts-expect-error
patchedFs.read[promisify.custom] = async (p: number, buffer: Buffer, ...args: Array<any>) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we do the same for write / readv / writev?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those other three aren't patched, so this problem doesn't occur.

If we wanted to patch those methods, then yeah we should do the promisify patching.

@Mike-Dax
Copy link
Contributor Author

I've added a test to validate the returned object shape. I tried to write a test that ran the unpatched fs.read then patched fs, and compared the two, but I couldn't seem to get access to the unpatched fs.read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants