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

File and stdio I/O #846

Merged
merged 1 commit into from
Sep 29, 2018
Merged

File and stdio I/O #846

merged 1 commit into from
Sep 29, 2018

Conversation

ry
Copy link
Member

@ry ry commented Sep 27, 2018

Depends on #838

@ry ry force-pushed the files branch 2 times, most recently from e8bc817 to 2253538 Compare September 27, 2018 08:21
Adds deno.stdin, deno.stdout, deno.stderr, deno.open(), deno.write(),
deno.read(), deno.Reader, deno.Writer, deno.copy().

Fixes denoland#721. tests/cat.ts works.
Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

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

Not super familiar with Tokio's async I/O model, so LGTM but take it with a grain of salt.
I didn't see any obvious mistakes -- the comments are more in the grandpa category.

export const stdout = new File(1);
export const stderr = new File(2);

// TODO This is just a placeholder - not final API.
Copy link
Member

Choose a reason for hiding this comment

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

😅
great

let mut table = FD_TABLE.lock().unwrap();
let maybe_repr = table.get_mut(&self.fd);
match maybe_repr {
None => panic!("bad fd"),
Copy link
Member

@piscisaureus piscisaureus Sep 29, 2018

Choose a reason for hiding this comment

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

It seems that this could actually happen, because an operation that closes the fs could overtake the async write.
(an argument for supporting some queuing, so it seems....)
But in the short term, maybe this shouldn't panic?

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to let things panic when we lack tests that demo a failure mode. That said - because the FD is checked first in the table - this shouldn’t ever happen...

Repr::FsFile(ref mut f) => f.poll_read(buf),
Repr::Stdin(ref mut f) => f.poll_read(buf),
Repr::Stdout(_) | Repr::Stderr(_) => {
panic!("Cannot read from stdout/stderr")
Copy link
Member

Choose a reason for hiding this comment

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

Ultimately, it's a bit more complicated than that. We'll need some support for the case where a parent process opens fd `4' in the child process, like node did for the IPC channel, which means that we probably shouldn't have super special treatment for fdD 0-2.

(not that I would suggest we make reading from stdout a first class citizen).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm - not sure I know about that FD 4 things - but this is just a first pass... let’s see what happens later on - I’m not necessarily against preventing allowing write to stdin or read from stdout if it makes sense..

@ry ry merged commit bcbbee7 into denoland:master Sep 29, 2018
ry added a commit to ry/deno that referenced this pull request Sep 29, 2018
- Adds deno.stdin, deno.stdout, deno.stderr, deno.open(), deno.write(),
  deno.read(), deno.Reader, deno.Writer, deno.copy() denoland#846
- Print 'Compiling' when compiling TS.
- Support zero-copy for writeFile() writeFileSync() denoland#838
- Fixes eval error bug denoland#837
- Make Deno multithreaded denoland#782
- console.warn() goes to stderr denoland#810
- Add deno.readlink()/readlinkSync() denoland#797
- Add --recompile flag denoland#801
- Use constructor.name to print out function type denoland#664
- Rename deno.argv to deno.args
- Add deno.trace() denoland#795
- Continuous benchmarks https://denoland.github.io/deno/
@ry ry mentioned this pull request Sep 29, 2018
ry added a commit that referenced this pull request Sep 29, 2018
- Adds deno.stdin, deno.stdout, deno.stderr, deno.open(), deno.write(),
  deno.read(), deno.Reader, deno.Writer, deno.copy() #846
- Print 'Compiling' when compiling TS.
- Support zero-copy for writeFile() writeFileSync() #838
- Fixes eval error bug #837
- Make Deno multithreaded #782
- console.warn() goes to stderr #810
- Add deno.readlink()/readlinkSync() #797
- Add --recompile flag #801
- Use constructor.name to print out function type #664
- Rename deno.argv to deno.args
- Add deno.trace() #795
- Continuous benchmarks https://denoland.github.io/deno/
hardfist pushed a commit to hardfist/deno that referenced this pull request Aug 7, 2024
Bumped versions for 0.299.0

Co-authored-by: bartlomieju <bartlomieju@users.noreply.github.com>
Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>
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

Successfully merging this pull request may close these issues.

2 participants