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

net: fix named pipe connect #5208

Merged
merged 5 commits into from
Nov 20, 2022
Merged

Conversation

satakuma
Copy link
Member

Closes: #5170

@satakuma satakuma marked this pull request as ready for review November 18, 2022 20:52
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Nov 18, 2022
@Darksonn
Copy link
Contributor

Can you please make sure that we have a test for both inbound = false and inbound = true, since this seems to be causing trouble?

@satakuma satakuma force-pushed the alice/fix-named-pipe branch from 75017ce to 425bb69 Compare November 19, 2022 13:37
@satakuma
Copy link
Member Author

I added a test with different inbound/outbound access settings. It fails (hangs) both on master and with my previous, flawed solution.

Comment on lines 357 to 358
// Wait for the server to call connect.
time::sleep(Duration::from_millis(10)).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't have tests that depend on timing. They are brittle, and make the test suite take a long time. 10 ms sleeps add up to seconds surprisingly quickly if you have lots of tests.

You can do something like this:

let (tx, rx) = tokio::sync::oneshot::channel();
let server = tokio::spawn(async move {
    let s = ServerOptions::new()
        .access_inbound(inb)
        .access_outbound(outb)
        .create(PIPE_NAME)?;

    let mut connect_fut = tokio_test::task::spawn(s.connect());
    assert!(connect_fut.poll().is_pending());

    let _ = tx.send(());

    connect_fut.await.unwrap();
});

// Wait for the server to call connect.
rx.await.unwrap();
let _ = ClientOptions::new().read(outb).write(inb).open(PIPE_NAME)?;

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks.

@Darksonn Darksonn enabled auto-merge (squash) November 20, 2022 20:36
@Darksonn Darksonn merged commit 2682c50 into tokio-rs:master Nov 20, 2022
@satakuma satakuma deleted the alice/fix-named-pipe branch November 20, 2022 21:07
carllerche added a commit that referenced this pull request Dec 5, 2022
 - net: fix Windows named pipe connect ([#5208])
 - io: support vectored writes for `ChildStdin` ([#5216])
 - io: fix `async fn ready()` false positive for OS-specific events ([#5231])

 ### Changed
 - runtime: `yield_now` defers task until after driver poll ([#5223])
 - runtime: reduce amount of codegen needed per spawned task ([#5213])
 - windows: replace `winapi` dependency with `windows-sys` ([#5204])

 [#5208]: #5208
 [#5216]: #5216
 [#5213]: #5213
 [#5204]: #5204
 [#5223]: #5223
 [#5231]: #5231
carllerche added a commit that referenced this pull request Dec 5, 2022
### Fixed
 - net: fix Windows named pipe connect ([#5208])
 - io: support vectored writes for `ChildStdin` ([#5216])
 - io: fix `async fn ready()` false positive for OS-specific events ([#5231])

 ### Changed
 - runtime: `yield_now` defers task until after driver poll ([#5223])
 - runtime: reduce amount of codegen needed per spawned task ([#5213])
 - windows: replace `winapi` dependency with `windows-sys` ([#5204])

 [#5208]: #5208
 [#5216]: #5216
 [#5213]: #5213
 [#5204]: #5204
 [#5223]: #5223
 [#5231]: #5231
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Dec 23, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.22.0` -> `1.23.0` |
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.22.0` -> `1.23.0` |

---

### Release Notes

<details>
<summary>tokio-rs/tokio</summary>

### [`v1.23.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.23.0): Tokio v1.23.0

[Compare Source](tokio-rs/tokio@tokio-1.22.0...tokio-1.23.0)

##### Fixed

-   net: fix Windows named pipe connect ([#&#8203;5208])
-   io: support vectored writes for `ChildStdin` ([#&#8203;5216])
-   io: fix `async fn ready()` false positive for OS-specific events ([#&#8203;5231])

##### Changed

-   runtime: `yield_now` defers task until after driver poll ([#&#8203;5223])
-   runtime: reduce amount of codegen needed per spawned task ([#&#8203;5213])
-   windows: replace `winapi` dependency with `windows-sys` ([#&#8203;5204])

[#&#8203;5208]: tokio-rs/tokio#5208

[#&#8203;5216]: tokio-rs/tokio#5216

[#&#8203;5213]: tokio-rs/tokio#5213

[#&#8203;5204]: tokio-rs/tokio#5204

[#&#8203;5223]: tokio-rs/tokio#5223

[#&#8203;5231]: tokio-rs/tokio#5231

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43MC4wIiwidXBkYXRlZEluVmVyIjoiMzQuNzAuMCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1687
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Strange freezing behavior of unidirectional Windows named pipes on single-threaded runtime
2 participants