Skip to content

uvio's read_stream() may not trigger task scheduling #9605

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

Closed
klutzy opened this issue Sep 29, 2013 · 7 comments
Closed

uvio's read_stream() may not trigger task scheduling #9605

klutzy opened this issue Sep 29, 2013 · 7 comments
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows

Comments

@klutzy
Copy link
Contributor

klutzy commented Sep 29, 2013

std::rt::uv::uvio::read_stream() calls StreamWatcher::read_start() with on_read callback containing scheduler.resume_blocked_task_immediately(task_cell.take());.

However, in some bad cases, read_start() does not call on_read, thus the task hangs.

Here is excerpt from libuv/src/win/stream.c:

int uv_read_start(uv_stream_t* handle, uv_alloc_cb alloc_cb,
    uv_read_cb read_cb) {
...
  if (handle->flags & UV_HANDLE_READING) {
    fprintf(stderr, "UV_HANDLE_READING. return UV_EALREADY");
    return UV_EALREADY;
  }

  if (!(handle->flags & UV_HANDLE_READABLE)) {
    fprintf(stderr, "!UV_HANDLE_READABLE. return UV_ENOTCONN");
    return UV_ENOTCONN;
  }

...
      err = uv_tcp_read_start((uv_tcp_t*)handle, alloc_cb, read_cb);
...
}

This blocks #8811's read_eof_twice_ip4() and read_eof_twice_ip6() tests on win32: second stream.read() triggers UV_HANDLE_READABLE condition.
The problem does not occur on unix since there is no check equivalent to UV_HANDLE_READABLE.

@klutzy
Copy link
Contributor Author

klutzy commented Sep 29, 2013

We currently ignore return value from uvll::read_start(). If this function returns != 0, the read_cb may not be called. (I'm not sure if it is promised by API design, since libuv API doesn't explain return value at all.)

@klutzy
Copy link
Contributor Author

klutzy commented Sep 30, 2013

I guessed uv_read_start() == 0 indicates cb_read callback is called, but it seems not for one case: On win32, uv_tty_read_start() (called by uv_read_start) can return 0 without callback trigger.

@brson
Copy link
Contributor

brson commented Oct 1, 2013

Let's handle the return value correctly.

@klutzy
Copy link
Contributor Author

klutzy commented Oct 2, 2013

@brson I have a dirty-stashed patch which calls read_cb by hand when nonzero comes. This solves some cases, but I'm still unclear if this is the best. What about uv_tty_read_start() mentioned above?

@klutzy
Copy link
Contributor Author

klutzy commented Oct 2, 2013

I now understand that return value really indicates whether it is correctly registered to event pool or not.

klutzy added a commit to klutzy/rust that referenced this issue Oct 4, 2013
@klutzy
Copy link
Contributor Author

klutzy commented Oct 4, 2013

std::rt::uv::net::StreamWatcher::write has similar issue: it assert_eq!s that return value is zero, but assertion failes for std::rt::io::net::tcp::write_close_ip* tests on win32.

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

This also fixes two tests of #8811.
@alexcrichton
Copy link
Member

This was closed by #9721

flip1995 pushed a commit to flip1995/rust that referenced this issue Oct 20, 2022
fix: uninlined_format_args shouldn't inline panic! before 2021ed

Before 2021 edition, `panic!("...")` was not treated as a format string.
Clippy autofix of `panic!("{}", foo)` into `panic!("{foo}")` is incorrect.

changelog: [`uninlined_format_args`]: Do not inline panic! macros before 2021 edition
flip1995 pushed a commit to flip1995/rust that referenced this issue Oct 20, 2022
Fix edition revision ui tests

rust-lang#9605 had me wondering how the edition revision tests were working for `manual_assert` but not for `@nyurik,` but it turns out `manual_assert`'s tests weren't working either. I checked how `rust-lang/rust` does it and apparently it comes down to whitespace, `//[rev] edition:X` works 😬

Removes the revisions from `match_wild_err_arm` as I couldn't find any edition dependant behaviour there

r? `@llogiq`

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows
Projects
None yet
Development

No branches or pull requests

3 participants