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

chore: fix windows CI #2597

Merged
merged 7 commits into from
Jul 13, 2020
Merged

chore: fix windows CI #2597

merged 7 commits into from
Jul 13, 2020

Conversation

carllerche
Copy link
Member

Trying to see if windows CI can be fixed.

@@ -601,6 +601,7 @@ rt_test! {
}

#[test]
#[cfg(not(windows))]
Copy link
Member Author

Choose a reason for hiding this comment

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

This test can't pass on windows. For now, I'm going to disable it. I will document why before completing the PR. I mostly want to see if CI passes.

@@ -221,7 +221,7 @@ impl OwnedWriteHalf {
impl Drop for OwnedWriteHalf {
fn drop(&mut self) {
if self.shutdown_on_drop {
let _ = self.inner.shutdown(Shutdown::Both);
let _ = self.inner.shutdown(Shutdown::Write);
Copy link
Member Author

Choose a reason for hiding this comment

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

Shutdown::Read has no defined portable behavior. This is why it doesn't pass on windows.

@carllerche carllerche requested a review from Darksonn June 8, 2020 22:46
@carllerche carllerche added the A-ci Area: The continuous integration setup label Jun 8, 2020
@carllerche carllerche marked this pull request as ready for review June 8, 2020 22:55
@Matthias247
Copy link
Contributor

I think this is now no longer has an equivalent behavior.

The idea behind shutting down the reading end was to emulate the same kind of behavior that e.g. socket APIs in Go and Java have: If you close the socket in those environments on one handle (e.g. a writing thread), it will unblock any operation blocked on receive. This helps if you have have one of those typical scenarios where there is a reader and a writer task and the reader is basically permanently blocked on the socket since it just reads in a loop. If it doesn't get woken by the close it will be stuck there until the peer also closes.

Now doing this with shutdown instead of close is not exactly equivalent, but nevertheless was intended to do that job.

If that behavior is removed then any application which already relies on that behavior would get broken. The wakeup could probably be implemented on top of the native socket by grabbing the read waker from somewhere when the writer is closed, and storing and checking an extra "closed", but it's certainly more work.

This would now get balanced again breaking the API.

@carllerche
Copy link
Member Author

shutdown(Read) has no portable behavior and the implementation as is did not work correctly across platforms.

Additionally, dropping a write half should only impact the write half of things, not the read. it is incorrect for Tokio to shutdown the read half when the write is dropped. This change is fixing a bug.

@carllerche
Copy link
Member Author

@Darksonn IMO the API needs to be either fixed by making the change in this PR or deprecated w/ the reason that it cannot be implemented in a portable way and done again in 0.3. Which route do you want to take?

@Darksonn
Copy link
Contributor

Considering that we are changing behavior on some OS with either option, I think it's ok to just fix it.

@carllerche carllerche merged commit 98e7831 into master Jul 13, 2020
@carllerche carllerche deleted the ci-win-hang branch July 13, 2020 02:26
hawkw added a commit that referenced this pull request Jul 20, 2020
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2572, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)

- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- io: fix panic in `AsyncReadExt::read_line` (#2541)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <[email protected]>
hawkw added a commit that referenced this pull request Jul 22, 2020
# 0.2.22 (July 2!, 2020)

### Fixes
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)
- io: fix panic in `AsyncReadExt::read_line` (#2541)

### Changes
- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

### Added
- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ci Area: The continuous integration setup A-tokio Area: The main tokio crate M-net Module: tokio/net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants