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

Server: require State to be Clone #644

Merged
merged 2 commits into from
Jul 17, 2020

Conversation

Fishrock123
Copy link
Member

Alternative to
#642

This approach is more flexible but requires the user ensure that their
state implements/derives Clone, or is wrapped in an Arc.

Co-authored-by: Jacob Rothstein hi@jbr.me

Notably while this has less new API surface, this requires more from user code.

examples/graphql.rs Outdated Show resolved Hide resolved
examples/middleware.rs Outdated Show resolved Hide resolved
src/server.rs Outdated Show resolved Hide resolved
@Fishrock123
Copy link
Member Author

I was on the edge about it, but i should have said in my OP: "Is non-Arc Clone" actually what a user would expect?"

I'm not sure it is personally? Like, your state will not actually be shared between anything at all if it's not an Arc - it will only contain the data set originally and set within any specific request. That isn't "shared" state in any sense. If this is really for shared state I'm not honestly sure this is the correct direction...

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jul 13, 2020

"Is non-Arc Clone" actually what a user would expect?"

I think so; this is not an uncommon thing to provide, and think it's entirely reasonable thing to allow people to implement. Certain performance sensitive applications may actually want this degree of control, and empowering them to implement this is useful.

For example: you could imagine someone only has State to pass config from the CLI into the app. Instead of putting it behind an atomic reference, it may just be faster to copy values. Not having an Arc at all is useful, and just slapping #[derive(Clone)] on State would be a great solution.

Alternative to
http-rs#642

This approach is more flexible but requires the user ensure that their
state implements/derives `Clone`, or is wrapped in an `Arc`.

Co-authored-by: Jacob Rothstein <hi@jbr.me>
@Fishrock123
Copy link
Member Author

Updated as per suggestions. 😄

Copy link
Member

@jbr jbr left a comment

Choose a reason for hiding this comment

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

lgtm! my only nit is that since the examples are didactic, we might want to set an example of doing

// examples/upload.rs
type State = Arc<TempDir>;
/*snip*/
.put(|req: Request<State>| async move {

or create a State struct, like https://github.com/jbr/tide-example/blob/master/src/main.rs#L7-L19.

@Fishrock123
Copy link
Member Author

Updated that example... i think there is a bit more work to do on the examples more generally outside of this PR.

@Fishrock123 Fishrock123 merged commit 261038d into http-rs:master Jul 17, 2020
@Fishrock123 Fishrock123 deleted the server-state-clone branch July 31, 2020 17:04
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.

None yet

3 participants