-
Notifications
You must be signed in to change notification settings - Fork 50
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
Stop log-forwarding thread on IO errors #133
Conversation
9e600c2
to
530c58e
Compare
530c58e
to
4722c7c
Compare
@rib we probably want to have this in so that I can test |
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.
Yeah I suppose it's a bit more faff, but it could be nice if we'd log some kind of clue if you hit a spurious error here. If you want to leave that for a separate follow up that's ok though.
Not required but it could also be nice to share a utility for spawning this stdio thread since the code is identical between the two backends.
4722c7c
to
a9985cf
Compare
Done.
Let's leave stopping+joining the thread for a followup, as we'd have to close stdin/stdout - hoping they haven't been
Done, good idea. The |
When `read_line()` starts returning `Err` the current `if let Ok` condition ignores those, likely causing the `loop` to spin indefinitely while this function keeps returning errors. Note that we don't currently store the join handle for this thread anywhere, so won't see the error surface either (just like how the join handle for the main thread is never checked). Perhaps we should call `log::error!()` to make the user aware that their IO logging has mysteriously terminated.
a9985cf
to
af34189
Compare
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.
Even though this was originally proposed in #145 I guess it makes sense to just name this thread here while creating a shared utility for spawning the stdio thread.
This looks good to me, thanks.
Yeah I hope @fornwall doesn't mind some preliminary conflict resolution... Though I can revert it. |
Absolutely no problem, I'll fix! |
Fixes #132
When
read_line()
starts returningErr
the currentif let Ok
condition ignores those, likely causing theloop
to spin indefinitely while this function keeps returning errors.Note that we don't currently store the join handle for this thread anywhere, so won't see the error surface either (just like how the join handle for the main thread is never checked). Perhaps we should call
log::error!()
to make the user aware that their IO logging has mysteriously terminated.