-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Handle uv_read_start
error
#9721
Conversation
@@ -147,7 +147,18 @@ impl StreamWatcher { | |||
data.read_cb = Some(cb); | |||
} | |||
|
|||
unsafe { uvll::read_start(self.native_handle(), alloc_cb, read_cb); } |
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.
This seems like a generic problem with our uv bindings. Do you know of the scope of the remaining callsites which need to handle this return value?
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'm working on uvll::write()
since it also blocks win32 tests.
I guess most code using this pattern:
do scheduler.deschedule_running_task_and_then |_, task| {
do call_uv_something_with_callback |_watcher, status| {
let scheduler: ~Scheduler = Local::take();
scheduler.resume_blocked_task_immediately(task_cell.take());
}
}
can be problematic if call_uv_something_with_callback()
is not aware of uv_something() != 0
.
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'd have to take a look myself, but it sounds like this needs more of a general refactoring than fixing on a case-by-case basis.
What it this currently blocking on windows? Is it basically error cases aren't handled correctly, or is the behavior much more serious than that? It's another one of those things where the runtime is in some serious need for some love right now, and it'd be sad to just pile more stuff on it instead of fixing the root causes while we still can.
That being said, if this is urgent, then this otherwise looks good to me.
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.
More explanation on tests:
read_eof_twice_ip4
and read_eof_twice_ip6
tests try to read socket which got EOF before. This occurs EOF (read_cb
is called with empty data) on unix, but on Windows it does not call read_cb
and just returns error.
This is due to libuv's win32 implementation. when libuv met EOF, it sets handle->flags &= ~UV_HANDLE_READABLE;
which means the handle is not usable. When read is called again, libuv sees the flag and returns error. I'm not sure why they programmed it differently...
So, I think we have to check return value of uvll::func()
for most cases for safety. Currently some routines have assertion e.g.
assert_eq!(0, uvll::write(req.native_handle(), self.native_handle(), [buf], write_cb));
however in some cases the assertion can be invalid in non-fatal case. This is why write_close_ip4
test fails on win32.
I'm currently aware of these two cases only. Some parts may be sufficient with assert_eq!
, but other parts need manual callback handling. I don't have concrete answer other than read/write until I'll look down all possibilities from libuv code.
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'm beginning to think that this probably isn't the right place to address these concerns, let's let this request through and then go back to check all the existing uv bindings.
Is there anything that still needs to be addressed here to get this merged? |
Add new lint [`let_underscore_future`] This closes rust-lang#9721 --- changelog: add new lint [`let_underscore_future`]
See #9605 for detailed information.
This also fixes two tests of #8811.