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.

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

Jump to bottom

sync: implement more traits for channel errors #5666

Merged
merged 4 commits into from
May 3, 2023
Merged

sync: implement more traits for channel errors #5666

merged 4 commits into from
May 3, 2023

Conversation

GilShoshan94
Copy link
Contributor

Closes #5664

Hi @Darksonn,

I did it, I looked at std::sync::mpsc::SendError and std::sync::mpsc::TrySendError implementations as a based to my commits.

I looked over all the Tokio's channel and found (in addition to the mpsc) a similar issue with the watch channel so I fixed it too.

There is one difference with the std code that I didn't do because it's adding a bond Send on T for the Error trait impl on their error and I don't see why it is required... (See in std::sync::mpsc impl<T: Send> Error for SendError and impl<T: Send> Error for TrySendError)

Do you think we need to impose T: Send for the Error impl ?

I ran the test on the whole repo on my machine and all the tests passed (and some where ignored).

@GilShoshan94
Update mpsc::error
5753c9b 
Update mpsc::error::{SendError<T>, TrySendError<T>, SendTimeoutError<T>} to match the errors from the std lib in their mpsc channel.

Removed bond <T: Debug>.
Add derived impl PartialEq, Eq, Clone and Copy (only if T implement them)
@GilShoshan94
Update watch::error
c452bdb 
Update watch::error::SendError<T> to match the error from the std lib in their mpsc channel (also a SendError).

Removed bond <T: Debug>.
Add derived impl PartialEq, Eq, Clone and Copy (only if T implement them)
@github-actions github-actions bot added the R-loom Run loom tests on this PR label May 2, 2023
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels May 2, 2023
@GilShoshan94
Copy link
Contributor Author

@Darksonn,

It's my first PR in such a big project !
All the CI tests passed.

I read the CONTRIBUTING document, but it is not clear to me if I was supposed to increase the SemVer (it's clearly a patch) or if it's something done only by the Tokio team only ?

Also there is this that I don't know what to do about:

Do you think we need to impose T: Send for the Error impl ?

@Darksonn
Copy link
Contributor

Darksonn commented May 2, 2023

The version number should only be modified in release PRs. You can find examples of those here. You shouldn't modify it in this PR.

The Send bound appears to be due to rust-lang/rust#23541 requiring all implementations of Error to be Send. Since that bound has been removed, it doesn't make sense to have it anymore. I expect that the standard library would accept a PR to remove the Send bound. You don't have to include it here.

Darksonn
Darksonn approved these changes May 3, 2023
View reviewed changes
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. Looks good to me.

@Darksonn Darksonn merged commit 61b68a8 into tokio-rs:master May 3, 2023
@Darksonn Darksonn changed the title mpsc and watch errors (SendError<T>...) without bond T:Debug sync: implement more traits for channel errors May 3, 2023
@GilShoshan94 GilShoshan94 mentioned this pull request May 3, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 15, 2023
@matthiaskrgr
Rollup merge of rust-lang#111134 - GilShoshan94:remove-send-bound-on-…
206c7a5 
…error, r=dtolnay

Remove unnecessary Send bound

Hi,

While working on a [PR on Tokio](tokio-rs/tokio#5666), I took inspiration from the std channel mpsc and stumbled on a `Send` bound for a `Error` impl.

Tokio's maintainer `@Darksonn` pointed out to me that `Error` used to required the `Send` bound long time ago (here rust-lang#23541).

In the meantime, the `Send` bound `Error` got removed (see rust-lang#21312 and rust-lang#23799).

So here a PR to removed this bound for `SendError<T>`, `TrySendError<T>` and `SendTimeoutError<T>`.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 16, 2023
@bors
Auto merge of rust-lang#111134 - GilShoshan94:remove-send-bound-on-er…
3ea9ad5 
…ror, r=dtolnay

Remove unnecessary Send bound

Hi,

While working on a [PR on Tokio](tokio-rs/tokio#5666), I took inspiration from the std channel mpsc and stumbled on a `Send` bound for a `Error` impl.

Tokio's maintainer `@Darksonn` pointed out to me that `Error` used to required the `Send` bound long time ago (here rust-lang#23541).

In the meantime, the `Send` bound `Error` got removed (see rust-lang#21312 and rust-lang#23799).

So here a PR to removed this bound for `SendError<T>`, `TrySendError<T>` and `SendTimeoutError<T>`.
RalfJung pushed a commit to RalfJung/miri that referenced this pull request May 16, 2023
@bors
Auto merge of #111134 - GilShoshan94:remove-send-bound-on-error, r=dt…
0b294c1 
…olnay

Remove unnecessary Send bound

Hi,

While working on a [PR on Tokio](tokio-rs/tokio#5666), I took inspiration from the std channel mpsc and stumbled on a `Send` bound for a `Error` impl.

Tokio's maintainer `@Darksonn` pointed out to me that `Error` used to required the `Send` bound long time ago (here rust-lang/rust#23541).

In the meantime, the `Send` bound `Error` got removed (see rust-lang/rust#21312 and rust-lang/rust#23799).

So here a PR to removed this bound for `SendError<T>`, `TrySendError<T>` and `SendTimeoutError<T>`.
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jul 6, 2023
@crapStone
Update Rust crate tokio to 1.29.1 (#1958)
03776c3 
This PR contains the following updates:

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

---

### Release Notes

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

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

[Compare Source](tokio-rs/tokio@tokio-1.29.0...tokio-1.29.1)

##### Fixed

-   rt: fix nesting two `block_in_place` with a `block_on` between (#&#8203;5837])

#&#8203;5837]: tokio-rs/tokio#5837

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

[Compare Source](tokio-rs/tokio@tokio-1.28.2...tokio-1.29.0)

Technically a breaking change, the `Send` implementation is removed from
`runtime::EnterGuard`. This change fixes a bug and should not impact most users.

##### Breaking

-   rt: `EnterGuard` should not be `Send` (#&#8203;5766])

##### Fixed

-   fs: reduce blocking ops in `fs::read_dir` (#&#8203;5653])
-   rt: fix possible starvation (#&#8203;5686], #&#8203;5712])
-   rt: fix stacked borrows issue in `JoinSet` (#&#8203;5693])
-   rt: panic if `EnterGuard` dropped incorrect order (#&#8203;5772])
-   time: do not overflow to signal value (#&#8203;5710])
-   fs: wait for in-flight ops before cloning `File` (#&#8203;5803])

##### Changed

-   rt: reduce time to poll tasks scheduled from outside the runtime (#&#8203;5705], #&#8203;5720])

##### Added

-   net: add uds doc alias for unix sockets (#&#8203;5659])
-   rt: add metric for number of tasks (#&#8203;5628])
-   sync: implement more traits for channel errors (#&#8203;5666])
-   net: add nodelay methods on TcpSocket (#&#8203;5672])
-   sync: add `broadcast::Receiver::blocking_recv` (#&#8203;5690])
-   process: add `raw_arg` method to `Command` (#&#8203;5704])
-   io: support PRIORITY epoll events (#&#8203;5566])
-   task: add `JoinSet::poll_join_next` (#&#8203;5721])
-   net: add support for Redox OS (#&#8203;5790])

##### Unstable

-   rt: add the ability to dump task backtraces (#&#8203;5608], #&#8203;5676], #&#8203;5708], #&#8203;5717])
-   rt: instrument task poll times with a histogram (#&#8203;5685])

#&#8203;5766]: tokio-rs/tokio#5766

#&#8203;5653]: tokio-rs/tokio#5653

#&#8203;5686]: tokio-rs/tokio#5686

#&#8203;5712]: tokio-rs/tokio#5712

#&#8203;5693]: tokio-rs/tokio#5693

#&#8203;5772]: tokio-rs/tokio#5772

#&#8203;5710]: tokio-rs/tokio#5710

#&#8203;5803]: tokio-rs/tokio#5803

#&#8203;5705]: tokio-rs/tokio#5705

#&#8203;5720]: tokio-rs/tokio#5720

#&#8203;5659]: tokio-rs/tokio#5659

#&#8203;5628]: tokio-rs/tokio#5628

#&#8203;5666]: tokio-rs/tokio#5666

#&#8203;5672]: tokio-rs/tokio#5672

#&#8203;5690]: tokio-rs/tokio#5690

#&#8203;5704]: tokio-rs/tokio#5704

#&#8203;5566]: tokio-rs/tokio#5566

#&#8203;5721]: tokio-rs/tokio#5721

#&#8203;5790]: tokio-rs/tokio#5790

#&#8203;5608]: tokio-rs/tokio#5608

#&#8203;5676]: tokio-rs/tokio#5676

#&#8203;5708]: tokio-rs/tokio#5708

#&#8203;5717]: tokio-rs/tokio#5717

#&#8203;5685]: tokio-rs/tokio#5685

</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:eyJjcmVhdGVkSW5WZXIiOiIzNi4wLjAiLCJ1cGRhdGVkSW5WZXIiOiIzNi4wLjAiLCJ0YXJnZXRCcmFuY2giOiJkZXZlbG9wIn0=-->

Co-authored-by: cabr2-bot <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1958
Reviewed-by: crapStone <[email protected]>
Co-authored-by: Calciumdibromid Bot <[email protected]>
Co-committed-by: Calciumdibromid Bot <[email protected]>
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Jul 18, 2023
@bors
Auto merge of #111134 - GilShoshan94:remove-send-bound-on-error, r=dt…
3142e41 
…olnay

Remove unnecessary Send bound

Hi,

While working on a [PR on Tokio](tokio-rs/tokio#5666), I took inspiration from the std channel mpsc and stumbled on a `Send` bound for a `Error` impl.

Tokio's maintainer `@Darksonn` pointed out to me that `Error` used to required the `Send` bound long time ago (here rust-lang/rust#23541).

In the meantime, the `Send` bound `Error` got removed (see rust-lang/rust#21312 and rust-lang/rust#23799).

So here a PR to removed this bound for `SendError<T>`, `TrySendError<T>` and `SendTimeoutError<T>`.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
@bors
Auto merge of #111134 - GilShoshan94:remove-send-bound-on-error, r=dt…
1ec3e59 
…olnay

Remove unnecessary Send bound

Hi,

While working on a [PR on Tokio](tokio-rs/tokio#5666), I took inspiration from the std channel mpsc and stumbled on a `Send` bound for a `Error` impl.

Tokio's maintainer `@Darksonn` pointed out to me that `Error` used to required the `Send` bound long time ago (here rust-lang/rust#23541).

In the meantime, the `Send` bound `Error` got removed (see rust-lang/rust#21312 and rust-lang/rust#23799).

So here a PR to removed this bound for `SendError<T>`, `TrySendError<T>` and `SendTimeoutError<T>`.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
@bors
Auto merge of #111134 - GilShoshan94:remove-send-bound-on-error, r=dt…
5f2cef5 
…olnay

Remove unnecessary Send bound

Hi,

While working on a [PR on Tokio](tokio-rs/tokio#5666), I took inspiration from the std channel mpsc and stumbled on a `Send` bound for a `Error` impl.

Tokio's maintainer `@Darksonn` pointed out to me that `Error` used to required the `Send` bound long time ago (here rust-lang/rust#23541).

In the meantime, the `Send` bound `Error` got removed (see rust-lang/rust#21312 and rust-lang/rust#23799).

So here a PR to removed this bound for `SendError<T>`, `TrySendError<T>` and `SendTimeoutError<T>`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Darksonn Darksonn Darksonn approved these changes

Assignees
No one assigned
Labels
A-tokio Area: The main tokio crate M-sync Module: tokio/sync R-loom Run loom tests on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Debug impl of mpsc::error::TrySendError<T> should not bound T: Debug
2 participants
@GilShoshan94 @Darksonn