Closed adrieng closed 3 weeks ago
Surprise deadlocks are definitely not reasonable, especially not exposing them to a newcomer to webdev! Thanks for the issue!
I've further simplified the example (in terms of triggering the inner code that is causing the problem) to
let () =
Dream.run
@@ Dream.logger
@@ Dream.sql_pool ~size:100 "sqlite3:db.sqlite"
@@ Dream.sql_sessions
@@ fun request ->
Dream.sql request @@ fun _ ->
Dream.sql request @@ fun _ ->
Dream.respond "unreachable"
What's happening here is that the Sqlite3 DB driver does not support concurrent connections AFAICT, so the inner call to Dream.sql
deadlocks, as the only available connection is already held by the outer call.
I think the more important question, unsimplifying the example, is do you need to have the Dream.set_session_field
call inside Dream.sql
for the correctness of your application? If so, the way the SQL sessions work might need to be reworked in Dream, so that they can either commit updates directly using a currently acquired connection, or can implicitly defer updates until the connection is released, depending on what is needed for the correctness of your application.
Assuming your application can still be correct if it calls Dream.set_session_field
outside Dream.sql
and you are able to work around the deadlock, it might be sufficient to just add a warning about this, to help catch it in development as quickly as possible. I've pushed that in PR #333. It might be too noisy, as non-Sqlite3 users might be getting away with re-entrant concurrent connections somehow somewhere. But I think we can find out in practice.
I have indeed changed my code to call Dream.set_session_field
outside of Dream.sql
after I understood the problem. I can imagine scenarios where the database and session data must be updated together atomically, but this was not an issue in my case.
Thanks a lot for the PR, and for your work on Dream in general.
Great! I will merge the PR for now to help mitigate this kind of situation partially, and also note bigger issues with updating the session atomically for future work.
If you could sketch out a scenario where the session has to be updated atomically, it would be helpful. But no need -- I'll try to come up with something compelling eventually :)
I made the mistake of calling
Dream.set_session_field
from the callback passed toDream.sql
. Since I had configured sessions to be stored in the SQL database,Dream.set_session_field
itself calledDream.sql
, immediately leading to a deadlock for this client. Below is a minimal reproducible example.As a newcomer to web development, I am not sure whether this behavior is considered reasonable or not. If it is, perhaps it should be mentioned in the documentation.