Skip to content

Server: require State to be Clone - #644

Merged
Fishrock123 merged 2 commits into
http-rs:masterfrom
Fishrock123:server-state-clone
Jul 17, 2020
Merged

Fishrock123 merged 2 commits into
http-rs:masterfrom
Fishrock123:server-state-clone

Conversation

@Fishrock123

Copy link
Copy Markdown
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.

Comment thread examples/graphql.rs Resolved
Comment thread examples/middleware.rs Resolved
Comment thread src/server.rs Resolved
@Fishrock123

Copy link
Copy Markdown
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

yoshuawuyts commented Jul 13, 2020 •

Copy link
Copy Markdown
Member

"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
Copy Markdown
Member Author

Updated as per suggestions. 😄

@jbr jbr left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

3 participants