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

windows: TcpStream::shutdown does not wake up blocking reads #121594

Open
lukas-code opened this issue Feb 25, 2024 · 1 comment
Open

windows: TcpStream::shutdown does not wake up blocking reads #121594

lukas-code opened this issue Feb 25, 2024 · 1 comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-bug Category: This is a bug. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@lukas-code
Copy link
Member

lukas-code commented Feb 25, 2024

Description

The docs for TcpStream::shutdown currently guarantee that a shutdown with Shutdown::Read must wake up pending operations immediately:

This function will cause all pending and future I/O on the specified portions to return immediately with an appropriate value (see the documentation of Shutdown).

The docs for Shutdown::Read explicitly says that currently blocked reads must return:

All currently blocked and future reads will return Ok(0).

However, that is currently not the case on *-pc-windows-* platforms. Instead, a currently blocked read will stay blocked forever when shutdown is called with Shutdown::Read in a different thread. The same also happens with Shutdown::Both.

This is causing the close_read_wakes_up test to fail spuriously due to a race condition, for example in #121523 (comment) and #120543 (comment). The test only passes if the shutdown happens before the read.

Repro

I tried this code:

use std::io::Read;
use std::net::{Ipv6Addr, Shutdown, TcpListener, TcpStream};
use std::thread;
use std::time::Duration;

fn main() {
    let listener = TcpListener::bind((Ipv6Addr::LOCALHOST, 0)).unwrap();
    let listener_addr = listener.local_addr().unwrap();
    let mut serverbound_stream = None;
    thread::scope(|scope| {
        // 1. `accept` and `connect` concurrently
        let _clientbound_stream = scope.spawn(|| {
            let (clientbound_stream, _) = listener.accept().unwrap();
            clientbound_stream
        });
        let serverbound_stream =
            serverbound_stream.insert(TcpStream::connect(listener_addr).unwrap());

        // 3. shutdown read during blocking read
        scope.spawn(|| {
            thread::sleep(Duration::from_secs(1)); // just to be sure
            serverbound_stream.shutdown(Shutdown::Read).unwrap();
        });

        // 2. blocking read
        let count = (&*serverbound_stream).read(&mut [0]).unwrap();
        assert_eq!(count, 0);
    });
}

I expected to see this happen: The .shutdown(Shutdown::Read) makes the blocking read return immediately with Ok(0).

Instead, this happened: The blocking read does not return, making the program hang indefinitely.

Meta

rustc --version --verbose:

rustc 1.78.0-nightly (381d69953 2024-02-24)
binary: rustc
commit-hash: 381d69953bb7c3390cec0fee200f24529cb6320f
commit-date: 2024-02-24
host: x86_64-unknown-linux-gnu
release: 1.78.0-nightly
LLVM version: 18.1.0

Tested on these platforms:

  • x86_64-pc-windows-msvc with Microsoft Windows Server 2019 (GitHub CI)
  • x86_64-pc-windows-gnu with wine 9.1 on Linux

Related

@lukas-code lukas-code added the C-bug Category: This is a bug. label Feb 25, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 25, 2024
@lukas-code
Copy link
Member Author

lukas-code commented Feb 25, 2024

I think there is no (simple) way to make the implementation conform to the docs, so we should probably just remove the guarantees about currently blocked operations from the docs and only talk about future operations.

Note that POSIX shutdown also only talks about subsequent operations and not currently blocked ones.

@rustbot label A-docs T-libs-api O-windows A-io -needs-triage

@rustbot rustbot added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 25, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 9, 2024
…ChrisDenton

fix `close_read_wakes_up` test

On windows, `shutdown` does not interrupt `read`, even though we document that it does (see rust-lang#121594).

The `close_read_wakes_up` test has a race condition and only passes on windows if the `shutdown` happens before the `read`. This PR ignores the test on windows adds a sleep to make it more likely that the `read` happens before the `shutdown` and the test actually tests what it is supposed to test on other platforms.

I'm submitting this before any docs changes, so that we can find out on what platforms `shutdown` actually works as documented.

r? `@ChrisDenton`
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Mar 10, 2024
fix `close_read_wakes_up` test

On windows, `shutdown` does not interrupt `read`, even though we document that it does (see rust-lang/rust#121594).

The `close_read_wakes_up` test has a race condition and only passes on windows if the `shutdown` happens before the `read`. This PR ignores the test on windows adds a sleep to make it more likely that the `read` happens before the `shutdown` and the test actually tests what it is supposed to test on other platforms.

I'm submitting this before any docs changes, so that we can find out on what platforms `shutdown` actually works as documented.

r? `@ChrisDenton`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
fix `close_read_wakes_up` test

On windows, `shutdown` does not interrupt `read`, even though we document that it does (see rust-lang/rust#121594).

The `close_read_wakes_up` test has a race condition and only passes on windows if the `shutdown` happens before the `read`. This PR ignores the test on windows adds a sleep to make it more likely that the `read` happens before the `shutdown` and the test actually tests what it is supposed to test on other platforms.

I'm submitting this before any docs changes, so that we can find out on what platforms `shutdown` actually works as documented.

r? `@ChrisDenton`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
fix `close_read_wakes_up` test

On windows, `shutdown` does not interrupt `read`, even though we document that it does (see rust-lang/rust#121594).

The `close_read_wakes_up` test has a race condition and only passes on windows if the `shutdown` happens before the `read`. This PR ignores the test on windows adds a sleep to make it more likely that the `read` happens before the `shutdown` and the test actually tests what it is supposed to test on other platforms.

I'm submitting this before any docs changes, so that we can find out on what platforms `shutdown` actually works as documented.

r? `@ChrisDenton`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-bug Category: This is a bug. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants