Implement request-state local cache #655

vhakulinen · 2018-06-06T17:58:07Z

Opening this PR for feedback. Based completely on @SergioBenitez's design.

I guess the next step is to extend the state crate to allow more relaxed bounds for T in the local_cache?

The cache needs to be wrapped in Rc so it gets Clone, but it was mentioned that in future Request might be send to another thread to be processed, so should we already use Arc instead?

Simple example usage https://gist.github.com/vhakulinen/fb84746fa102761c0683f694523a47cd.

Closes #654.

SergioBenitez · 2018-06-06T17:59:24Z

Please add some doc-tests, unit tests, and integration tests.

vhakulinen · 2018-06-16T11:41:47Z

Sorry, didn't mean to close this PR.

SergioBenitez · 2018-06-20T08:56:18Z

Depending on "wall time" for the test is error prone. Instead, lets use something like two request guards each which access the same pair of AtomicUsizes. One of the atomics should be incremented in the closure that initializes the cached value, and the other should be incremented outside of the closure. Thus, after calling a handler that makes use of both request guards, the atomic accessed inside the initialization closure should have a value of one while the other should have a value of two.

In pseudocode, this is:

#[get("/")]
fn handler(Guard1, Guard2) { .. }

impl FromRequest for {Guard1, Guard2} {
    req.guard::<State<Atomics>>()?.first.increment();
    req.local_cache(|req| req.guard::<State<Atomics>>()?.second.increment());
}

fn main() {
    rocket::ignite().manage(Atomics);
}

#[test]
fn test() {
    client.get("/").dispatch();
    assert_eq!(client.rocket().state::<Atomics>().unwrap().first.value(), 2);
    assert_eq!(client.rocket().state::<Atomics>().unwrap().second.value(), 1);
}

SergioBenitez · 2018-06-29T02:32:47Z

examples/request_state_local_cache/src/main.rs

+    type Error = ();
+
+    fn from_request(req: &'a Request<'r>) -> request::Outcome<Self, ()> {
+        req.guard::<State<Atomics>>()?.first.fetch_add(1, Ordering::Relaxed);


Just call the Guard1 request guard here.

SergioBenitez · 2018-06-29T02:33:04Z

examples/request_state_local_cache/src/main.rs

+    second: AtomicUsize,
+}
+
+struct Guard1();


No need for () here or when it's constructed.

SergioBenitez · 2018-06-29T02:34:22Z

examples/request_state_local_cache/src/tests.rs

+    let client = Client::new(rocket()).unwrap();
+    client.get("/").dispatch();
+
+    assert!(client.rocket().state::<Atomics>().unwrap().first.load(Ordering::Relaxed) == 2, "First is two");


Store the reference to Atomics in a variable to clean these assert!s up:

let atomics = client.rocket().state::<Atomics>().unwrap(); assert_eq!(atomics.uncached.load(Ordering::Relaxed), 2); assert_eq!(atomics.cached.load(Ordering::Relaxed), 2);

SergioBenitez · 2018-06-29T02:34:35Z

examples/request_state_local_cache/src/main.rs

+#[cfg(test)] mod tests;
+
+struct Atomics {
+    first: AtomicUsize,


Let's rename these cached and uncached.

SergioBenitez · 2018-06-29T02:35:02Z

examples/request_state_local_cache/src/main.rs

+
+fn rocket() -> rocket::Rocket {
+    rocket::ignite()
+        .manage(Atomics{


Instead, derive Default for Atomics and call Atomics::default() here.

SergioBenitez · 2018-06-29T02:37:21Z

core/lib/src/request/request.rs

+    /// ```
+    pub fn local_cache<T, F>(&self, f: F) -> &T
+        where T: Send + Sync + 'static,
+              F: FnOnce(&Request) -> T {


On second thought: why are we passing self to F? The caller already has a handle to request, so they can use it if they need to. I think this should probably be F: FnOnce() -> T.

examples/request_state_local_cache/src/main.rs

+
+
+#[get("/")]
+fn index(_g1: Guard1, _g2: Guard2) {


SergioBenitez · 2018-06-29T02:41:33Z

examples/request_state_local_cache/src/main.rs

+    type Error = ();
+
+    fn from_request(req: &'a Request<'r>) -> request::Outcome<Self, ()> {
+        req.guard::<State<Atomics>>()?.first.fetch_add(1, Ordering::Relaxed);


Pull out the atomics state loading:

let atomics = req.guard::<State<Atomics>>()?; atomics.uncached.fetch_add(1, Ordering::Relaxed); req.local_cache(|| atomics.cached.fetch_add(1, Ordering::Relaxed)); Guard1

This change wasn't made.

SergioBenitez · 2018-06-29T02:41:35Z

examples/request_state_local_cache/src/main.rs

+
+    fn from_request(req: &'a Request<'r>) -> request::Outcome<Self, ()> {
+        req.guard::<State<Atomics>>()?.first.fetch_add(1, Ordering::Relaxed);
+        req.local_cache(|req| {


This is saving a value of type () in the cache. I suppose that's fine, but let's at least leave a comment indicating this.

vhakulinen · 2018-06-29T18:00:27Z

Updated the PR based on feedback.

I've been wondering whether or not the use of trait would be better for local_cache instead of FnOnce. I've yet to come up with compelling example, where such situation would be favorable - I guess it would only make it a bit more explicit that the cache is per type.

SergioBenitez · 2018-06-29T20:31:01Z

examples/request_state_local_cache/src/tests.rs

+    client.get("/").dispatch();
+
+    let atomics = &client.rocket().state::<Atomics>().unwrap();
+    assert!(atomics.uncached.load(Ordering::Relaxed) == 2, "First is two");


No need for the string comment. Also, use assert_eq. Same for the one below.

SergioBenitez · 2018-06-29T20:31:12Z

examples/request_state_local_cache/src/tests.rs

+    let client = Client::new(rocket()).unwrap();
+    client.get("/").dispatch();
+
+    let atomics = &client.rocket().state::<Atomics>().unwrap();


I don't think you need the & there.

Hmh, when I tried without &, I had to de-reference ((*atomics).cached...) the value when I wanted to access it, for some reason. Seems to work without it now.

SergioBenitez · 2018-06-29T20:31:39Z

examples/request_state_local_cache/src/main.rs

+        Success(Guard2)
+    }
+}
+


Kill this extra empty line.

SergioBenitez · 2018-06-29T20:31:58Z

examples/request_state_local_cache/src/main.rs

+    type Error = ();
+
+    fn from_request(req: &'a Request<'r>) -> request::Outcome<Self, ()> {
+        req.local_cache(|| {


Please make the change in the previous comment.

I see I commited my experiments.

SergioBenitez · 2018-07-08T01:58:06Z

Merged in 97c6b3a. Hoorah! Thanks so much for this!

Implement request-state local cache

e03a875

Examples and tests for request state local cache

8fa1cb9

SergioBenitez mentioned this pull request Jun 12, 2018

Add MTLS Support #657

Closed

vhakulinen closed this Jun 16, 2018

vhakulinen reopened this Jun 16, 2018

Improve request state local cache example tests

a5d55cd

SergioBenitez requested changes Jun 29, 2018

View reviewed changes

vhakulinen added 2 commits June 29, 2018 20:41

Improve request local cache test based on feedback

63674f9

Don't pass &Request to local_cache

26dc36b

SergioBenitez requested changes Jun 29, 2018

View reviewed changes

Minor tweaks to request state local cache tests

1f7ce3b

SergioBenitez approved these changes Jul 8, 2018

View reviewed changes

SergioBenitez closed this Jul 8, 2018

SergioBenitez added the pr: merged This pull request was merged manually. label Jul 8, 2018

SergioBenitez mentioned this pull request Aug 13, 2018

Can we protect routes for API endpoints and do rate limiting in Rocket? #679

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement request-state local cache #655

Implement request-state local cache #655

vhakulinen commented Jun 6, 2018

SergioBenitez commented Jun 6, 2018

vhakulinen commented Jun 16, 2018

SergioBenitez commented Jun 20, 2018

SergioBenitez Jun 29, 2018

SergioBenitez Jun 29, 2018

SergioBenitez Jun 29, 2018

SergioBenitez Jun 29, 2018

SergioBenitez Jun 29, 2018

SergioBenitez Jun 29, 2018

This comment was marked as resolved.

SergioBenitez Jun 29, 2018

SergioBenitez Jun 29, 2018

SergioBenitez Jun 29, 2018

vhakulinen commented Jun 29, 2018

SergioBenitez Jun 29, 2018

SergioBenitez Jun 29, 2018

vhakulinen Jun 30, 2018 •

edited

Loading

SergioBenitez Jun 29, 2018

SergioBenitez Jun 29, 2018

vhakulinen Jun 30, 2018

SergioBenitez commented Jul 8, 2018

Implement request-state local cache #655

Implement request-state local cache #655

Conversation

vhakulinen commented Jun 6, 2018

SergioBenitez commented Jun 6, 2018

vhakulinen commented Jun 16, 2018

SergioBenitez commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vhakulinen commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vhakulinen Jun 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez commented Jul 8, 2018

vhakulinen Jun 30, 2018 •

edited

Loading