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

Handle uv_read_start error #9721

Merged
merged 3 commits into from
Oct 17, 2013
Merged

Handle uv_read_start error #9721

merged 3 commits into from
Oct 17, 2013

Conversation

klutzy
Copy link
Contributor

@klutzy klutzy commented Oct 4, 2013

See #9605 for detailed information.

This also fixes two tests of #8811.

@@ -147,7 +147,18 @@ impl StreamWatcher {
data.read_cb = Some(cb);
}

unsafe { uvll::read_start(self.native_handle(), alloc_cb, read_cb); }
Copy link
Member

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?

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'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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

@brson
Copy link
Contributor

brson commented Oct 17, 2013

Is there anything that still needs to be addressed here to get this merged?

bors added a commit that referenced this pull request Oct 17, 2013
See #9605 for detailed information.

This also fixes two tests of #8811.
@bors bors closed this Oct 17, 2013
@bors bors merged commit ade57d9 into rust-lang:master Oct 17, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 21, 2022
Add new lint [`let_underscore_future`]

This closes rust-lang#9721
---

changelog: add new lint [`let_underscore_future`]
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.

4 participants