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(io): handle missing source in stdin #534

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dignifiedquire
Copy link
Contributor

Currently io::stdin() when used with lines().fuse() from
futures, will panic on the first poll. This adds a test for this
case, as well as a potential fix.

Currently `io::stdin()` when used with `lines().fuse()` from
`futures`, will panic on the first poll. This adds a test for this
case, as well as a potential fix.
let res = match source.result() {
Some(res) => res,
None => {
return Poll::Pending;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if this is the correct fix, but it does fix both the test case and when using stdin in my program to read from it

@dignifiedquire
Copy link
Contributor Author

it seems the missing stdin on CI is failing the tests, any suggestions on how to adjust such that it actually tests the right thing?

@HippoBaro
Copy link
Member

The fix looks good, but your new unit test is failing in CI. I'll try to find some time to reproduce locally to see what's going on.

This new test relies on timing, though, and generally, that's best avoided.

@dignifiedquire
Copy link
Contributor Author

it doesn’t actually rely on timing, just the fact that nothing gets written to stdin and that it is available

@HippoBaro HippoBaro force-pushed the master branch 2 times, most recently from 616409a to d480c63 Compare February 27, 2022 03:04
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