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
Add global state #20
Add global state #20
Conversation
|
||
pub struct GlobalCounter; | ||
|
||
impl Key for GlobalCounter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to see the typemap
crate traded for a hand-written shio::util::typemap
module.
- Use
FnvHasher
or a no-op hasher ( mtsr/rust-typemap@8f4e422 ) over std hashing algo (would need to profile but I'm sure one of these is far faster for our use case here) - It wouldn't be much code as we only use a subset of
typemap
(which is already tiny) - We could add
default impl<T> Key for T { type Value = T }
which I think would greatly improve ergonomics of.manage
(e.g. a connection pool could be passed in directly without a wrapping type).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea, maybe in another PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely another PR. Just something I thought of and wanted to make note of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do that, should be ready today or tomorrow.
EDIT: done (#21)
@@ -59,6 +67,26 @@ impl Context { | |||
pub fn try_get<K: Key>(&self) -> Option<&K::Value> { | |||
self.state.get::<K>() | |||
} | |||
|
|||
/// Gets a value from the global context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually thinking a fallback. E.g. context.get::<K>()
would try local state first and then fallback to global state. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be convenient, but I think we should keep the get_global
and try_get_global
methods, because it's two disjoint typemaps. Maybe have a get_local
and try_get_local
that don't try to fallback, and adding fallback to get
and try_get
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we add FromRequest
(for request guards) there'll be a 3rd "virtual" typemap as well.
I'd rather error/warn on a global -> local override and maintain the typemaps have an empty intersection in the API.
We have 4 ways to get state into handlers, and each one has a very distinct set of use cases.
- "Dynamic" (
FromRequest
)- Current authorized user (or 403)
- Database connection (on-demand connection checkout)
- "Global" shared state (from
Shio::manage
)- Database connection pool
slog
logger
- "Local" state (adding into the state from somewhere in the middleware chain). I can't actually think of a use for this off the top of my head.
iron
used this for persisting data betweenbefore
andafter
handlers in aChain
but ergonomics for "around" middleware have gotten good enough that its not necessary. - Per-handler shared state (in a structure). I can't think of a good use case for this either because I have it drilled in my head that stateful requests are bad but I feel this is sure to be useful to someone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For "Local" state, it's a workaround for passing state. You have one example into shio: the router::Parameters
that is inserted by Router
.
For per-handler shared state, it's useful only if it's read-only when the server run, like the Router
. It can also be useful for countering requests, for another example.
The "Global" shared state is (if we ignore ergonomics a moment) handled by the current PR.
The FromRequest
of Rocket is getting data from Request
state, and transform it.
In fact, it's mostly context.get<State>().get()
(pseudo-code).
For the Database connection, I think of something like context.from_request<DbConn>()
, with DbConn
implementing FromRequest
.
For authorization, it's a little more complex, but the only part missing is the FromRequest
for DbConn
and Cookies
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For "Local" state, it's a workaround for passing state. You have [...]
Ha. That's right. I'm using it as a "hack" there because the Router is completely decoupled. Forgot about that.
The key benefit of FromRequest
is state is "on-demand". Routes that need it, get it. This matters for a DbConn
(where traditionally each request gets one whether it needs it or not).
I still see FromRequest
being used from context.get( .. )
. It'd have the lowest priority.
I need to sit down and really think through error propagation though. FromRequest
or dynamic items generally need to have error behaviour (e.g. database checkout -> 503 or user auth -> 403).
Enough rambling. Sorry, I can get carried away with a thought. Do you mind just doing the fallback and we can open an issue to discuss if/how to add get_global
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem for doing the fallback.
EDIT: doing the fallback is a breaking change, because the global typemap requires Send + Sync
trait bound.
…into Meralis40-global_typemap
Pulled updates from master |
I made a few changes.
context.get::<T>(); // per-request
context.shared().get::<T>(); // shared
Awesome work as always getting this started. |
Implements a global read-only typemap.
This permit to have global data for all requests, that are requested on demand. See the new example, global_state, for an usage.
The new methods are :
Shio::manage
: add a new element to global context.Context::get_global
andContext::try_get_global
: similar toContext::get
andContext::try_get
, but in global context instead of request context.This closes #13, and also provide the storage part of #12.