Send the state to the Session backend.

[StephenWakely]

This is for #467.

It sends the State to the persist_session, read_session and drop_session methods of the Backend. This will be useful for additional backends, such as a Redis backend, that will need to access a connection that will be stored in the State.

[msrd0]

Thanks for the PR!

Can you please fix the issues reported by travis - that is, the test code needs to be updated and after that you'll need to run rustfmt.

[codecov]

Codecov Report

Merging #468 (d046c44) into master (fe7d15b) will increase coverage by 0.51%.
The diff coverage is 69.23%.

❗ Current head d046c44 differs from pull request most recent head 204cc25. Consider uploading reports for the commit 204cc25 to get more accurate results

@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
+ Coverage   85.01%   85.52%   +0.51%     
==========================================
  Files         110      109       -1     
  Lines        5612     5582      -30     
==========================================
+ Hits         4771     4774       +3     
+ Misses        841      808      -33     

Impacted Files	Coverage Δ
gotham/src/middleware/session/mod.rs	78.87% <56.52%> (-0.27%)	⬇️
gotham/src/middleware/session/backend/memory.rs	90.29% <87.50%> (-0.62%)	⬇️
gotham/src/service/mod.rs	90.56% <0.00%> (-9.44%)	⬇️
gotham/src/test.rs	72.13% <0.00%> (-1.21%)	⬇️
examples/sessions/custom_data_type/src/main.rs	90.90% <0.00%> (-0.62%)	⬇️
gotham/src/router/builder/draw.rs	87.91% <0.00%> (-0.52%)	⬇️
gotham/src/plain/test.rs	79.56% <0.00%> (ø)
gotham_derive/src/lib.rs	0.00% <0.00%> (ø)
gotham/src/pipeline/mod.rs	98.07% <0.00%> (ø)
... and 27 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe7d15b...204cc25. Read the comment docs.

[StephenWakely]

I've now modified it so the Backend functions returned futures. This should allow for more flexibility when defining different backends.

[msrd0]

Sorry for taking so long to review this - I believe this type signature is incorrect? Without any lifetime information, the GetSessionFuture type needs to live for 'static, which it will not do if it accessed the state in an async context. I think this needs to be changed to something like

persist_session<'fut, 'a: 'fut, 'b: 'fut, 'c: 'fut>(
    &'a self,
    state: &'b State,
    identifier: SessionIdentifier,
    content: &'c [u8]
) -> Pin<Box<GetSessionFuture + 'fut>>

Alternatively, it might make sense to let #[async_trait] deal with the lifetimes.

[StephenWakely]

No worries, thanks for reviewing. I'll try to get to these shortly.

The last time I used #[async_trait] it resulting in the rust compiler not being able to produce any decent error messages for any errors in the code that implemented those traits. We ended up changing back to returning Futures instead. Do you know if this problem has been resolved? I would be hesitant to use it unless it has..

[msrd0]

The only time I used #[async_trait] was to run cargo expand on it to see how it worked because I didn't want my proc macro to invoke another proc macro. So I don't know if this has been changed or has ever been a problem. However, I feel like the code would probably be more readable if we don't introduce this many lifetimes, but if you run into any incorrect/unhelpful error messages, just write the code the explicit way. I'm happy with both solutions.

[msrd0]

Same as above

[msrd0]

Same as above

[msrd0]

Do you think you can get this PR ready? I'd like to include it in the next release. Let me know if you need any help.

[StephenWakely]

Hi, really sorry, I haven't had the chance to get to this. Life has taken over. I don't think I will get the chance to look at it any time soon either. I'm happy for someone else to take this on if they are willing, or we can just leave it and I will get to at it some point, whenever that may be!

[msrd0]

No problem. I'm going to merge this as-is as it should work for most use cases, e.g. pulling a database connection out of the state and then enter an async block in which that connection is being used, and we can always improve it in the future.