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

Exceed reader limits #11530

Closed
wants to merge 6 commits into from
Closed

Exceed reader limits #11530

wants to merge 6 commits into from

Conversation

nbaum
Copy link
Contributor

@nbaum nbaum commented Jun 2, 2015

Possible fix for #10282, #10655.

This introduces readbytes(cmd::AsyncStream). Is it bad form to introduce new methods in a bugfix?

No tests. Sorry.

@nbaum
Copy link
Contributor Author

nbaum commented Jun 2, 2015

The tests fail at line 263 of repl.jl, output = readall(master). Probably this is related to master being a TTY.

Annoyingly, if I write to STDOUT just before the readall, the test passes...

@nbaum
Copy link
Contributor Author

nbaum commented Jun 2, 2015

I believe this has exposed something that was going wrong anyway.

The libuv read callback is being invoked with a negative nread: -5 for me, perhaps for everyone. It looks like it happens instead of an invocation with nread == UV_EOF.

The callback notifies readnotify waiters that an error occurred, but doesn't notify_error closenotify waiters. It forceably closes the stream, which then causes the close callback to be invoked: then the closenotify waiters are notified, but not told there was an error.

I don't know why the read callback is being invoked with a negative nread.

@nbaum
Copy link
Contributor Author

nbaum commented Jun 2, 2015

The error is UV_EIO. I believe it relates to the interaction between the TTY master and its slaves.

This SO question says that closing all the slaves on a TTY causes reading from the master to fail with EIO.

It suggests keeping a "spare" slave open, but if we do that readall never returns because the master never closes. I've adjusted the test to use readavailable, and then close the slave descriptor, which works.

The updated PR also changes the callback handler to notify close-waiters. Should that actually be in a separate PR, or is it okay in this one?

Aside: It looks like writing to stdout hides the problem because by the time the write has finished, the subprocess has already exited and closed the slave so readbytes doesn't ever wait for the readnotify condition and it never sees the error. The error state isn't stored anywhere permanent, so it looks like we can never know it happened. That feels like a bug.

@nbaum
Copy link
Contributor Author

nbaum commented Jun 3, 2015

I can't make make test fail on my x86_64 system here. I don't know what to do about this.

@vtjnash
Copy link
Member

vtjnash commented Jun 3, 2015

i'm looking into possible fixes for this

vtjnash added a commit that referenced this pull request Jun 3, 2015
, fix #10655 (closes #11530)

this also makes the read throttle more intelligent so that it doesn't
get tripped up by wait_nb requests for more than READ_BUFFER_SZ bytes
vtjnash added a commit that referenced this pull request Jun 3, 2015
, fix #10655 (closes #11530)

this also makes the read throttle more intelligent so that it doesn't
get tripped up by wait_nb requests for more than READ_BUFFER_SZ bytes
vtjnash added a commit that referenced this pull request Jun 4, 2015
, fix #10655 (closes #11530)

this also makes the read throttle more intelligent so that it doesn't
get tripped up by wait_nb requests for more than READ_BUFFER_SZ bytes
vtjnash added a commit that referenced this pull request Jun 4, 2015
, fix #10655 (closes #11530)

this also makes the read throttle more intelligent so that it doesn't
get tripped up by wait_nb requests for more than READ_BUFFER_SZ bytes
@vtjnash vtjnash closed this Jun 4, 2015
vtjnash added a commit that referenced this pull request Jun 6, 2015
, fix #10655 (closes #11530)

this also makes the read throttle more intelligent so that it doesn't
get tripped up by wait_nb requests for more than READ_BUFFER_SZ bytes
amitmurthy added a commit that referenced this pull request Jan 11, 2016
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