Dream.sql, reentrancy and sessions

[adrieng]

I made the mistake of calling Dream.set_session_field from the callback passed to Dream.sql. Since I had configured sessions to be stored in the SQL database, Dream.set_session_field itself called Dream.sql, immediately leading to a deadlock for this client. Below is a minimal reproducible example.

let main request =
  Dream.sql request (fun _ ->
      let%lwt () = Dream.set_session_field request "will" "deadlock" in
      Dream.respond "unreachable")

let () =
  Dream.run @@ Dream.logger @@ Dream.sql_pool "sqlite3:db.sqlite"
  @@ Dream.sql_sessions @@ Dream.router [Dream.get "/" main]

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.

[aantron]

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.

[adrieng]

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.

[aantron]

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.

[aantron]

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 :)