Skip to content

Handle uv_read_start error - #9721

Merged
bors merged 3 commits into
rust-lang:masterfrom
klutzy:uv-net-read-fix
Oct 17, 2013
Merged

Handle uv_read_start error#9721
bors merged 3 commits into
rust-lang:masterfrom
klutzy:uv-net-read-fix

Conversation

@klutzy

Copy link
Copy Markdown
Contributor

See #9605 for detailed information.

This also fixes two tests of #8811.

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
Contributor

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.
@borsbors closed this Oct 17, 2013
@bors
bors merged commit ade57d9 into rust-lang:masterOct 17, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 21, 2022
Add new lint [`let_underscore_future`]
This closesrust-lang#9721
---
changelog: add new lint [`let_underscore_future`]
Sign up for freeto 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

@klutzy@brson@alexcrichton@bors