-
Notifications
You must be signed in to change notification settings - Fork 28
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(shim-deno/std): Fix std not to use FsFile #98
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @hyp3rflow! That's annoying that occurs in Node and I'm able to reproduce it. I think we need to do a few more changes to get this merged though.
} | ||
}); | ||
return deferred.then((result) => { | ||
process.stdin.pause(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone does multiple read
calls at once before the first is complete, would this pause the second call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that code, yes. So I changed the logic to handle multiple calls correctly.
p.fill(0); | ||
process.stdin.resume(); | ||
process.stdin.once("readable", () => { | ||
const data = process.stdin.read(p.length) ?? process.stdin.read(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could falling back to read()
cause more data to be read than what's in the buffer and then for p.set(data)
to fail below with a RangeError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think no.
The optional size argument specifies a specific number of bytes to read. If size bytes are not available to be read, null will be returned unless the stream has ended, in which case, all of the data remaining in the internal buffer will be returned.
If the size argument is not specified, all of the data contained in the internal buffer will be returned.
According to the documentation, if buffer has data more than p.length
bytes, it doesn't fall back to stdin.read()
.
but buffer has data less than p.length
bytes or is empty, stdin.read()
will return all of the data contained in the buffer which is always less than p.length
bytes.
Thanks for the kind review, @dsherret! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @hyp3rflow!
}); | ||
return (prev = curr); | ||
} as T; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
process.stdin.on("error", onerror); | ||
process.stdin.once("readable", () => { | ||
process.stdin.off("error", onerror); | ||
const data = process.stdin.read(p.length) ?? process.stdin.read(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think actually it's possible that between the process.stdin.read(p.length)
and process.stdin.read()
calls that stdin could receive more bytes than are in the buffer. Let's resolve it in a separate PR. I'll open an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #99
If node's "process" module is imported, accessing stdin by
fs.read
throwsEAGAIN
error: resource temporarily unavailable.So I re-implemented std interface using
process.stdin
,process.stdout
,process.stderr
.resolves #97