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

im: Relax Send + Sync bounds #63

Merged
merged 2 commits into from
Nov 11, 2024
Merged

im: Relax Send + Sync bounds #63

merged 2 commits into from
Nov 11, 2024

Conversation

jplatte
Copy link
Owner

@jplatte jplatte commented Oct 13, 2024

No description provided.

@zzorba zzorba mentioned this pull request Oct 14, 2024
@zzorba
Copy link

zzorba commented Oct 14, 2024

Should eyeball/src/subscriber/async_lock.rs also be updated to use the new ReusableBoxFuture, so we can drop the dependency on tokio-util all together?

@@ -0,0 +1,161 @@
// Copy-pasted from https://docs.rs/tokio-util/latest/src/tokio_util/sync/reusable_box.rs.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add the license, or create an ATTRIBUTION file?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Is that some sort of standard? I didn't find anything immediately.

I felt when writing this that the comment at the top should be sufficient attribution for copying a small file out of a permissively-licensed project. I suppose it wouldn't hurt to note the original license. I don't think adding the license anywhere is necessary though, since MIT allows redistributing stuff under a different license IIRC (I am not a lawyer, of course).

Also, I plan to get rid of this module again in the future anyways, once tokio-rs/tokio#6908 is merged and another release is made it should be possible to write a self-referential future by hand that can be stored in the VectorSubscriberStream without going through dyn Future. It's a bit tricky though, and I haven't had any focus time for that sort of stuff lately. Let me know if you're interested in helping out 😉

@jplatte
Copy link
Owner Author

jplatte commented Nov 6, 2024

Should eyeball/src/subscriber/async_lock.rs also be updated to use the new ReusableBoxFuture, so we can drop the dependency on tokio-util all together?

No, I don't think any other code should be updated. The custom ReusableBoxFuture requires extra unsafe impl Sends to work in async contexts where futures need to be Send.

@jplatte jplatte marked this pull request as ready for review November 6, 2024 18:54
@jplatte
Copy link
Owner Author

jplatte commented Nov 6, 2024

Not sure when I'll get to implementing this in another way, though I would like to. Probably makes sense to merge this as-is for now (potentially with light adjustments for attribution).

@jplatte jplatte requested a review from Hywan November 7, 2024 19:04
Copy link
Collaborator

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Overall I think it's a good step forward. Looking for the tokio's PR to be merged. Well done.

@jplatte jplatte merged commit e218ce8 into main Nov 11, 2024
6 checks passed
@jplatte jplatte deleted the jplatte/send-sync branch November 11, 2024 16:56
@jplatte jplatte changed the title Relax Send + Sync bounds im: Relax Send + Sync bounds Nov 11, 2024
Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Nov 13, 2024
…nown`.

This patch updates `eyeball-im` and `eyeball-im-util` to integrate
jplatte/eyeball#63. With this new feature, we
can have a single implementation of `ObservableMap` (instead of 2: one
for all targets, one for `wasm32-u-u`). It makes it possible to get
`Client::rooms_stream` available on all targets now.
Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Nov 13, 2024
…nown`.

This patch updates `eyeball-im` and `eyeball-im-util` to integrate
jplatte/eyeball#63. With this new feature, we
can have a single implementation of `ObservableMap` (instead of 2: one
for all targets, one for `wasm32-u-u`). It makes it possible to get
`Client::rooms_stream` available on all targets now.
Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Nov 13, 2024
…nown`.

This patch updates `eyeball-im` and `eyeball-im-util` to integrate
jplatte/eyeball#63. With this new feature, we
can have a single implementation of `ObservableMap` (instead of 2: one
for all targets, one for `wasm32-u-u`). It makes it possible to get
`Client::rooms_stream` available on all targets now.
Hywan added a commit to matrix-org/matrix-rust-sdk that referenced this pull request Nov 13, 2024
…nown`.

This patch updates `eyeball-im` and `eyeball-im-util` to integrate
jplatte/eyeball#63. With this new feature, we
can have a single implementation of `ObservableMap` (instead of 2: one
for all targets, one for `wasm32-u-u`). It makes it possible to get
`Client::rooms_stream` available on all targets now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants