chore(webapp): Bump axum-login dependency to 0.13.1

[holzeis]

Not being super optimistic, but maybe the latest axum-login release fixes the issue with the randomly unresponsive webapp (#1938)

It looks like axum-login had issues with deadlocks in the past, but with tower-sessions those deadlock should have been fixed. Bumping from 0.12 to 0.13.1 also bumps the tower-session dependency. So this might help 🤞.

[bonomat]

sending to merge to give this a try right away.

[maxcountryman]

Hey @holzeis, sorry to drop in unannounced: I was looking at the impl of the session store here and noticed you all using a synchronous lock.

Maybe I'm misunderstanding here, but is this meant to be used in production? If so, I would be concerned that a synchronous lock within a store might directly lead to e.g. deadlocks.

Is there a reason to not use tokio::sync::RwLock instead?

[luckysori]

Hey @holzeis, sorry to drop in unannounced: I was looking at the impl of the session store here and noticed you all using a synchronous lock.

Maybe I'm misunderstanding here, but is this meant to be used in production? If so, I would be concerned that a synchronous lock within a store might directly lead to e.g. deadlocks.

Is there a reason to not use tokio::sync::RwLock instead?

Hey, @maxcountryman! Thanks for taking the time to look at our code and offer some advice.

I think you are absolutely right. At some point we spent a lot of time auditing the code base for instances of blocking code in an async context, but it seems like this has slipped code review.

Thanks again!

[luckysori]

@maxcountryman: I've been giving it a bit more thought and I now think this code is actually fine! Maybe you can correct me if I'm wrong though.

I am pretty sure the common advice is to avoid using async Mutex/RwLock unless strictly necessary¹². That is, if you need to hold it across an await point. The reason given is that async locks are considerably slower.

As far as I can tell, we are not actually holding the RwLock across an await point here.

What do you think?

Footnotes

1. https://tokio.rs/tokio/tutorial/shared-state. ↩

2. https://draft.ryhl.io/blog/shared-mutable-state. ↩

[maxcountryman]

While you're absolutely correct you generally don't need async locks, this is an example of the use case tokio calls out for async locks:

The primary use case for the async mutex is to provide shared mutable access to IO resources such as a database connection. ¹

Granted, we have a HashMap here, but the session store is used as though it is non-blocking and same principle should apply.

In your second link, Alice mentions:

In my experience, the best way to avoid the above issue is to never lock the mutex in async code in the first place. Instead, you define non-async methods on the wrapper struct and lock the mutex there. Then, you call those non-async methods from your async code.

Emphasis mine. Observe that our blocking code is inside the async methods of the session store API and so we might be leading ourselves into trouble.

More specifically multiple tasks may load and save but this is impossible when we've blocked on a synchronous lock.

Footnotes

1. https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html#which-kind-of-mutex-should-you-use ↩

[Restioson]

We could be causing it ourselves in delete_expired, since we lock the sessions for reading and then for writing later without dropping the guard - we are going to look into this imminently but I'm just putting a pin in it here so I don't forget!