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: add with_shared_state() #642

Closed
wants to merge 1 commit into from

Conversation

Fishrock123
Copy link
Member

Allows true state sharing for multiple tide server applications without Arc<Arc<State>>.

See discord discussion: https://discordapp.com/channels/598880689856970762/649056551835009097/731205374832541746

Allows true state sharing for multiple tide server applications without `Arc<Arc<State>>`.
@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jul 11, 2020

I can't open the discord link so I'm not sure what the motivation is — but guessing it seems like we want to have access to Arc<State> somehow?

Not opposed to always have require State to be Arcd when passed to the with_state constructor — or perhaps it's even better to require State: Clone which would provide the same guarantees in a more direct way.

edit: I'm a bit hesitant to add another constructor; it seems like it intends to work around deficiencies in with_state, so I'd prefer we attempt to address those before we consider adding more constructors.

@Fishrock123
Copy link
Member Author

Yeah, the idea is to be able to have more than one server share the same state without passing an Arc, which would become Arc<Arc<>> and then passed to middleware as an Arc<State>.

I think this was the intention to begin with but probably was mistaken?

Does State: Clone prevent us from having an Arc internally that is different from a user-passed Arc?

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jul 11, 2020

State: Clone indeed means we would no longer need to have an internal Arc since the state can be freely cloned already. with_state(Arc::new(state)) would be valid.

One downside however is that depending on how State is constructed, Arc may show up in the Server's type signature — but I think this is a worthwhile tradeoff.

@Fishrock123
Copy link
Member Author

I think that was part of the stated issue though, that it's annoying to have to deal with an Arc<> in the middleware when it's probably going to be anyways?

@yoshuawuyts
Copy link
Member

I mean in some cases it may very well be an Arc<RwLock> or perhaps only some fields need to be wrapped in an Arc<Mutex> for the whole struct to simply be #[derive (Clone)]. There are patterns we can introduce people to that extend to Rust as a whole.

I think State: Clone is probably the best way ahead here.

@Fishrock123
Copy link
Member Author

Fishrock123 commented Jul 11, 2020

Uhhh, I'm not sure that is possible. impl<State> Read for Request<State> requires DerefMut...

tide/src/request.rs

Lines 551 to 559 in 3778706

impl<State> Read for Request<State> {
fn poll_read(
mut self: Pin<&mut Self>,
cx: &mut Context<'_>,
buf: &mut [u8],
) -> Poll<io::Result<usize>> {
Pin::new(&mut self.req).poll_read(cx, buf)
}
}

Non-working patch at https://github.com/Fishrock123/tide/tree/server-state-clone

@Fishrock123
Copy link
Member Author

So @jbr got the above to compile using pin-project-lite: https://gist.github.com/jbr/b0fd2964eb57a1d8d512e4d167ad6554

However I think that Request probably shouldn't hold the State. See #643

Fishrock123 added a commit to Fishrock123/tide that referenced this pull request Jul 12, 2020
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 <[email protected]>
Fishrock123 added a commit to Fishrock123/tide that referenced this pull request Jul 12, 2020
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 <[email protected]>
Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

The outcome I'd like to see is for there to only be a single Server::with_state method that takes State where State: Clone. The Arc in the function signatures seems off, and I don't think we need multiple methods here.

Comment on lines +288 to +329
/// Create a new Tide server with shared application scoped state.
///
/// Shared application scoped state is useful for storing items,
/// including across multiple tide applications.
///
/// # Examples
///
/// ```no_run
/// # use async_std::task::block_on;
/// # fn main() -> Result<(), std::io::Error> { block_on(async {
/// #
/// use std::sync::Arc;
/// use tide::Request;
///
/// /// The shared application state.
/// struct State {
/// name: String,
/// }
///
/// // Define a new instance of the state.
/// let state = Arc::new(State {
/// name: "Nori".to_string()
/// });
///
/// // Initialize the application with state.
/// let mut app1 = tide::with_shared_state(state.clone());
/// let mut app2 = tide::with_shared_state(state.clone());
/// app2.at("/name").get(|req: Request<State>| async move {
/// Ok(format!("Hello, {}!", &req.state().name))
/// });
/// app1.at("/hello").nest(app2);
/// app1.listen("127.0.0.1:8080/hello/name").await?;
/// #
/// # Ok(()) }) }
/// ```
pub fn with_shared_state<State>(state: Arc<State>) -> server::Server<State>
where
State: Send + Sync + 'static,
{
Server::with_shared_state(state)
}

Copy link
Member

Choose a reason for hiding this comment

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

Now that State: Clone this method is no longer needed.

Suggested change
/// Create a new Tide server with shared application scoped state.
///
/// Shared application scoped state is useful for storing items,
/// including across multiple tide applications.
///
/// # Examples
///
/// ```no_run
/// # use async_std::task::block_on;
/// # fn main() -> Result<(), std::io::Error> { block_on(async {
/// #
/// use std::sync::Arc;
/// use tide::Request;
///
/// /// The shared application state.
/// struct State {
/// name: String,
/// }
///
/// // Define a new instance of the state.
/// let state = Arc::new(State {
/// name: "Nori".to_string()
/// });
///
/// // Initialize the application with state.
/// let mut app1 = tide::with_shared_state(state.clone());
/// let mut app2 = tide::with_shared_state(state.clone());
/// app2.at("/name").get(|req: Request<State>| async move {
/// Ok(format!("Hello, {}!", &req.state().name))
/// });
/// app1.at("/hello").nest(app2);
/// app1.listen("127.0.0.1:8080/hello/name").await?;
/// #
/// # Ok(()) }) }
/// ```
pub fn with_shared_state<State>(state: Arc<State>) -> server::Server<State>
where
State: Send + Sync + 'static,
{
Server::with_shared_state(state)
}

@@ -167,7 +167,7 @@ impl Default for Server<()> {
}

impl<State: Send + Sync + 'static> Server<State> {
/// Create a new Tide server with shared application scoped state.
/// Create a new Tide server with application scoped state.
Copy link
Member

Choose a reason for hiding this comment

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

Application state is and always was shared within the application. Now that State: Clone it's true both locally to applications, and between applications.

Suggested change
/// Create a new Tide server with application scoped state.
/// Create a new Tide server shared with application scoped state.

@Fishrock123
Copy link
Member Author

Closing in favor of #644

Fishrock123 added a commit to Fishrock123/tide that referenced this pull request Jul 16, 2020
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 <[email protected]>
Fishrock123 added a commit to Fishrock123/tide that referenced this pull request Jul 16, 2020
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 <[email protected]>
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