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

Remove redundant Sync bound #319

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Remove redundant Sync bound #319

merged 1 commit into from
Sep 16, 2024

Conversation

evyatark2
Copy link
Contributor

This PR removes a redundant Sync bound on the future that is returned by f in serve_fn()

Futures generally shouldn't be Sync. This caused errors when working with functions whose return type is impl Future<...> + Send

@GlenDC
Copy link
Member

GlenDC commented Sep 16, 2024

Thank you @evyatark2 for this first contribution of yours to Rama!
As we are gearing up towards 0.2, patches like these are very welcome!

Futures generally shouldn't be Sync

Thanks for that advice, it does make sense. Will take this into account.

@GlenDC GlenDC merged commit 51ad98b into plabayo:main Sep 16, 2024
33 checks passed
@evyatark2
Copy link
Contributor Author

evyatark2 commented Sep 16, 2024

You welcome!

On a side note, I was wondering if it would be welcome to use the experimental feature RTN?

If you prefer to use stable rust, would it still be welcome to conditionally use RTN by introducing a new cargo feature like unstable?

@GlenDC
Copy link
Member

GlenDC commented Sep 16, 2024

I have played with that feature before in an early version of rama and/or tower-async.
However I have since not really felt the need for it.

Can you elaborate on why you are needing that, e.g. what are you missing in rama
that would require having this availability?

@evyatark2
Copy link
Contributor Author

evyatark2 commented Sep 16, 2024

It would be nice to have non-Send futures so I can use tokio's LocalSet to use shared data in a single thread without using a Mutex/RwLock

@GlenDC
Copy link
Member

GlenDC commented Sep 16, 2024

Would being able to mutate the State from Context<State> also work for you? As that is what I'm going to support soon, the work is almost finished for this.

@evyatark2
Copy link
Contributor Author

It was me on Discord :)

That would solve what I had trouble with, but making the futures non-Send is a separate issue.

@GlenDC
Copy link
Member

GlenDC commented Sep 16, 2024

I am not really inclined to make that work, no.
As that seems like a big change if you want to facilitate both stable and unstable. Or am I wrong?

@evyatark2
Copy link
Contributor Author

It would be a big change in the sense that it is a lot of boilerplate with #[cfg(feature = "unstable")] sprinkled all over the place.
I can go ahead and implement it and then you can review it and decide if it would be a nightmare to maintain or not.

@GlenDC
Copy link
Member

GlenDC commented Sep 17, 2024

It would be a big change in the sense that it is a lot of boilerplate with

That's what I mean indeed

I can go ahead and implement it and then you can review it and decide if it would be a nightmare to maintain or not.

I do not want to stop inovation, so I do not want to right out say no to this.
However as to avoid you putting in a lot of work that might not lead to an acceptance,
perhaps first do so for one area / module which will be enough to show how it will look like
when extrapolated to the rest of the code base. This way I also start to get a feel of it and
we can take it from there perhaps?

That might mean that the codebase doesn't compile / check at that moment due to many errors. That's ok,
it's mostly just to see your idea in action without having to go all the way already.

@evyatark2
Copy link
Contributor Author

Sure! I'll go right ahead

@evyatark2 evyatark2 deleted the remove-sync branch September 18, 2024 07:24
@evyatark2
Copy link
Contributor Author

evyatark2 commented Sep 18, 2024

Because of syntax parity between stable rust and nightly rust, it seems like currently it is not possible to support this feature in the way I wanted to. Guess we'll just wait for it to stabilize

@GlenDC
Copy link
Member

GlenDC commented Sep 18, 2024

Fair enough. Sorry to hear that. I did play with this syntax before. You can probably find a very old commit of Rama where I was using that RTN. At that time rama was still unstable-only. At that time I was waiting until either type Foo = impl Bar would become stable or -> impl Bar. The latter came to be.

For now it's however a bit of a restricted way to work with all those implicit bounds that only work easy if you just enforce it always. I understand however that it overly restricts legit use cases like yours.

I want to make clear however that I want to continue to evolve and develop rama alongside how Rust and its ecosystem evolves. As such there's a banner that says there are no current plants to go to a "1.0". That's not because I like to break code nilly-willy. Because once we have a first release (0.2) I will be more mindful of that. However, it does mean that we do need significant breaking changes as the world around us matures, and we mature with it. Feedback from people and contributors like yourself, and help, is also very welcome and will ensure to contribute to that.

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.

2 participants