Deadlock when using both read and write in the same handler

[Ptrskay3]

Having read an issue in axum-login I realized that it's really easy to get a deadlock with the current API of this crate. Since ReadableSession and WritableSession both using the same underlying resource behind an RwLock.

A simple axum handler to reproduce:

async fn deadlock(mut _write_session: WritableSession, _read_session: ReadableSession) {}

and this can be solved if you release the lock guard

async fn deadlock(mut _write_session: WritableSession, read_session: ReadableSession) {
    // do the read here..
    drop(read_session);
    // write operations get to run here..
}

This issue becomes especially awkward and more subtle when this becomes an implicit bound, just like here. AuthContext needs a write access, but also there's a session read in the same handler. The developer must be familiar with the implementation details of these structs to know to release the read guard.

I don't really know how to solve this with the current API, but I'm experimenting..

[czocher]

Why is Session behind a RWLock? Is the underlying implementation not thread-safe @maxcountryman?

Also from my quick glance at async_sessionit seems there's no division between a readable and writable version as implemented by this crate.

Maybe removing this division and the RWLock is a solution for this issue?

[Ptrskay3]

Why is Session behind a RWLock? Is the underlying implementation not thread-safe @maxcountryman?

Also from my quick glance at async_sessionit seems there's no division between a readable and writable version as implemented by this crate.

Maybe removing this division and the RWLock is a solution for this issue?

We've been iterating on this previously here. I personally avoid this issue by using a wrapper over async-session and this middleware, but we discussed this in #5 that this solution is kind of unfortunate and ugly. That being said, async-session's API is probably stable enough.

[maxcountryman]

I'm certainly open to improving and changing this.

The reason we have to use interior mutability is because the underlying Session implementation has its own Clone semantics which unfortunately blow away the cookie value (this means we can't clone it directly if we want things like regenerate to work). And because we need Send and Sync to provide the extension, we're limited to a lock or similar if we use the current approach.

[czocher]

Seems like not Cloneing the cookie_value was intentional. I suggest to create a new RFC-like issue to plan a new breaking change to fix the problem mentioned here and chart a new roadmap for the interface of this crate. What do you think @maxcountryman, @Ptrskay3?

In reality a simpler change to just fix this problem would possibly be to use some kind of reentrant/recursive lock. There's no recursive Mutex or RWLock implementation available ATM for async contexts and in reality needing a reentrant lock usually means there's some architectural problem in place (which there is, hence this task).

[Ptrskay3]

Yes, I knew that Clone on cookie_value was intentional. Because of we discussed this earlier and went down the interior mutability way I tried to present this as a non-API breaking thing, but it seems it might not be possible (or I don't know how to solve it).

I'm certainly not against a new API. A fairly simple solution is what I mentioned above (the wrapper over async_session). Did you have the chance to look at it?

[czocher]

@Ptrskay3 yes, seems reasonable in my opinion.

[maxcountryman]

Another thing to point out here is that it's unclear how often users are going to run into this in practice. It's a bigger issue with axum-login because of the way that library implicitly leverages WritableSession. The reason I mention this is because there's also an opportunity to consider if that library would be a better place to address this. (Of course, other libraries that use axum-sessions in a similar manner would potentially run into the same issue, so that's worth pointing out.)

Additionally, it may be worth investigating with async-session: what would they intend library authors do in situations like this? While the semantics of Clone make sense for direct consumption (it's a nice way to enforce certain properties of the session lifetime through the type system) it's unclear to me if that holds for libraries that consume async-session. Were there some escape hatch for this behavior, then interior mutability wouldn't be needed at all.

[czocher]

@maxcountryman correct me if I'm wrong, but I think the WritableSession allows both for read and write operations? If yes then maybe just changing the name from WritableSession to ReadableWritableSession or RWSession (or just Session) and specifically either forbidding a situation when someone requests a ReadableSession and RWSession at the same time or clearly stating in the docs the "gotchas" in this case (the required drop) would solve this situation for now.

If there's some way to forbid this situation in the code - a compilation error or even a runtime panic in the worst case - this issue is solved without any major changes to the API. We'd just need to change the docs slightly.

[Ptrskay3]

I think it's a fairly common thing to both read from and write to the session in the same handler. A straight forward examples are OAuth or Two-factor authentication. In my opinion the root cause of this issue is in this repository, and this is where it should be addressed - that's why I opened the issue here.

RWSession seems to be a good idea, but I think it doesn't solve the issue with axum-login: the library's AuthContext extractor still holding onto the lock, and the users needs to be aware of that if they want to use either ReadableSession or WriteableSession.

[LeoniePhiline]

I would propose replacing the ReadableSession and WritableSession extractors by just using the session handle.

The handle is added as Extension to the request anyway and can be extracted:

https://github.com/LeoniePhiline/axum-csrf-sync-pattern/blob/df341e8c2a9aeb2df8f62338394419828c6f6221/src/lib.rs#L151-L162

Guards should only be acquired as long as they are needed, and dropped afterwards.

Or did I miss anything in my CSRF synchronizer token middleware? The write guard is dropped before calling the inner service.
However, the middleware is of course awaiting many times between its start and its passing the request on to the inner service. Is there any chance this will cause a lock, e.g. in parallel requests using the same session? I would even think the session write lock should be held in that case, to avoid race conditions.

[maxcountryman]

I would propose replacing the ReadableSession and WritableSession extractors by just using the session handle.

This is possible, but I'm not sure how obvious the API is. The tradeoff is the caller needs to directly manage the guard. This might be fine (it does make it explicit which seems to be part of the problem of abstraction here).

To your point, this is already possible with the current implementation: applications can choose to use the SessionHandle instead.

[LeoniePhiline]

I would imagine directly managing guards is something quite common for developers working with parallelism or concurrency. Acquiring and dropping guards is standard Rust.

Providing read or write guards instead of the handle is a big footgun.

You if you want more abstraction than the bare handle, then you could also provide get/set/remove methods which handle locking and unlocking for the user.

To your point, this is already possible with the current implementation: applications can choose to use the SessionHandle instead.

Yes obviously. ;)
I did show that I am so sign the handle directly. My intention was to say that this usage should be documented and the currently documented guard extractors be deprecated.

Crates like axum-login need to treat the handle such that multiple extractors do not deadlock. I.e. hold any guards as short as possible or expose them to the user to acquire and drop.

[maxcountryman]

Crates like axum-login need to treat the handle such that multiple extractors do not deadlock.

It's important to note that axum-login will obscure the lock regardless of which extractor is used. So I'm not sure this is an argument to use SessionHandle directly. Additionally, the extractors aren't really hiding the lock so much as automating acquiring it; callers still have a guard as they would if they called the methods directly on SessionHandle.

If we could avoid a lock altogether, that would materially improve the ergonomics in my view. However, due to the way async-session has implemented their session type, that's challenging. I do think it would be interesting to hear from those crate authors: what's their recommendation for situations like this?

[maxcountryman]

Folks on this thread, I started a separate discussion regarding async-session: I'm now wondering if it might make sense to fork the crate. I'd love feedback over there. 😃

[maxcountryman]

Hi folks, I've just published tower-sessions 0.1.0.

It does away with the readable and writable pattern and moves towards a pattern whereby an inner state is held behind a mutex. This is the same pattern other successful tower middleware use, like tower-cookies. Importantly, this lock is only ever held during an operation over the inner hash map state.

It also brings the implementation of the session into the crate itself. This means we'll no longer be dependent on external changes in order to address issues as they might arise.

I suspect this design is far more robust than our current design and my hope is that we can migrate the vast majority of axum-sessions users to this new crate.

Please let me know what you all think.

[LeoniePhiline]

I'm all for it!

I'm absolutely ready to migrate https://github.com/LeoniePhiline/axum-csrf-sync-pattern to tower-sessions