search

plabayo / rama

modular service framework to move and transform network packets
https://ramaproxy.org
Apache License 2.0
187 stars 21 forks source link

Remove redundant `Sync` bound #319

Closed evyatark2 closed 2 months ago

evyatark2 commented 2 months ago

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 commented 2 months ago

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.

evyatark2 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

It was me on Discord :)

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

GlenDC commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Sure! I'll go right ahead

evyatark2 commented 2 months ago

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 commented 2 months ago

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.