Skip to content

Handle uv_read_start error #9721

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 17, 2013
Merged

Handle uv_read_start error #9721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a402fb2
std::rt::uv::net: Handle `read_start` error
klutzy Oct 4, 2013
8aadcd4
std::rt: Add NotConnected to IoErrorKind
klutzy Oct 4, 2013
ade57d9
rt::io::net::tcp: Fix eof_twice tests on Win32
klutzy Oct 4, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/libstd/rt/io/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,7 @@ pub enum IoErrorKind {
Closed,
ConnectionRefused,
ConnectionReset,
NotConnected,
BrokenPipe,
PathAlreadyExists,
PathDoesntExist,
Expand All @@ -386,6 +387,7 @@ impl ToStr for IoErrorKind {
Closed => ~"Closed",
ConnectionRefused => ~"ConnectionRefused",
ConnectionReset => ~"ConnectionReset",
NotConnected => ~"NotConnected",
BrokenPipe => ~"BrokenPipe",
PathAlreadyExists => ~"PathAlreadyExists",
PathDoesntExist => ~"PathDoesntExist",
Expand Down
26 changes: 20 additions & 6 deletions src/libstd/rt/io/net/tcp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,6 @@ mod test {
}

#[test]
#[ignore(cfg(windows))] // FIXME #8811
fn read_eof_twice_ip4() {
do run_in_mt_newsched_task {
let addr = next_test_ip4();
Expand All @@ -321,8 +320,16 @@ mod test {
let mut buf = [0];
let nread = stream.read(buf);
assert!(nread.is_none());
let nread = stream.read(buf);
assert!(nread.is_none());
do read_error::cond.trap(|e| {
if cfg!(windows) {
assert_eq!(e.kind, NotConnected);
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 sad that this happens on windows and not unix. I'd be more in favor of just swallowing all errors instead of testing for some errors on some platforms and none on others. It still seems icky to swallow errors though...

Copy link
Member

Choose a reason for hiding this comment

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

Do you know why we're not raising an error unix, and only on windows? Is it because on unix when libuv calls the callback from the second read that there's no error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'm not sure if it is really intended, so reported the behavior at joyent/libuv#948.

} else {
fail2!();
}
}).inside {
let nread = stream.read(buf);
assert!(nread.is_none());
}
}

do spawntask {
Expand All @@ -334,7 +341,6 @@ mod test {
}

#[test]
#[ignore(cfg(windows))] // FIXME #8811
fn read_eof_twice_ip6() {
do run_in_mt_newsched_task {
let addr = next_test_ip6();
Expand All @@ -349,8 +355,16 @@ mod test {
let mut buf = [0];
let nread = stream.read(buf);
assert!(nread.is_none());
let nread = stream.read(buf);
assert!(nread.is_none());
do read_error::cond.trap(|e| {
if cfg!(windows) {
assert_eq!(e.kind, NotConnected);
} else {
fail2!();
}
}).inside {
let nread = stream.read(buf);
assert!(nread.is_none());
}
}

do spawntask {
Expand Down
1 change: 1 addition & 0 deletions src/libstd/rt/uv/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ pub fn uv_error_to_io_error(uverr: UvError) -> IoError {
EACCES => PermissionDenied,
ECONNREFUSED => ConnectionRefused,
ECONNRESET => ConnectionReset,
ENOTCONN => NotConnected,
EPIPE => BrokenPipe,
err => {
rtdebug!("uverr.code {}", err as int);
Expand Down
15 changes: 13 additions & 2 deletions src/libstd/rt/uv/net.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rt::uv::uvll;
use rt::uv::uvll::*;
use rt::uv::{AllocCallback, ConnectionCallback, ReadCallback, UdpReceiveCallback, UdpSendCallback};
use rt::uv::{Loop, Watcher, Request, UvError, Buf, NativeHandle, NullCallback,
status_to_maybe_uv_error};
status_to_maybe_uv_error, vec_to_uv_buf};
use rt::io::net::ip::{SocketAddr, Ipv4Addr, Ipv6Addr};
use vec;
use str;
Expand Down Expand Up @@ -147,7 +147,18 @@ impl StreamWatcher {
data.read_cb = Some(cb);
}

unsafe { uvll::read_start(self.native_handle(), alloc_cb, read_cb); }
let ret = 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.


if ret != 0 {
// uvll::read_start failed, so read_cb will not be called.
// Call it manually for scheduling.
call_read_cb(self.native_handle(), ret as ssize_t);
}

fn call_read_cb(stream: *uvll::uv_stream_t, errno: ssize_t) {
Copy link
Member

Choose a reason for hiding this comment

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

This is unfortunate :( Is it because the read_cb function is extern below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Just to not put #[fixed_stack_segment] to read_start.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to put fixed_stack_segment on read_start, it's abut to call an FFI function in uvll::read_start anyway.

#[fixed_stack_segment]; #[inline(never)];
read_cb(stream, errno, vec_to_uv_buf(~[]));
}

extern fn alloc_cb(stream: *uvll::uv_stream_t, suggested_size: size_t) -> Buf {
let mut stream_watcher: StreamWatcher = NativeHandle::from_native_handle(stream);
Expand Down
2 changes: 2 additions & 0 deletions src/libstd/rt/uv/uvll.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ pub mod errors {
pub static EACCES: c_int = -4093;
pub static ECONNREFUSED: c_int = -4079;
pub static ECONNRESET: c_int = -4078;
pub static ENOTCONN: c_int = -4054;
pub static EPIPE: c_int = -4048;
}
#[cfg(not(windows))]
Expand All @@ -63,6 +64,7 @@ pub mod errors {
pub static EACCES: c_int = -libc::EACCES;
pub static ECONNREFUSED: c_int = -libc::ECONNREFUSED;
pub static ECONNRESET: c_int = -libc::ECONNRESET;
pub static ENOTCONN: c_int = -libc::ENOTCONN;
pub static EPIPE: c_int = -libc::EPIPE;
}

Expand Down