SQL pool is broken : there is one new pool per request

[F-Loyer]

The SQL pool allocates a new pool with each request. Then many simultaneous access make too many connexions.

The fix is easy:

in src/sql/sql.ml, replace

let sql_pool ?size url =
let pool_cell = ref None in 

By

let pool_cell = ref None 
let sql_pool ?size url =

then there is a unique pool_cell.

Note, I don’t understand the need of a Message passing, just the use of ref_cell could get the job done. But I haven’t try (race conditions ?)

[paurkedal]

One could also argue that the current version is correct and that SQL middlewares should be stored globally, as the example in the doc-comment does (anonymously), instead of being re-created on each request.

[F-Loyer]

One could also argue that the current version is correct and that SQL middlewares should be stored globally, as the example in the doc-comment does (anonymously), instead of being re-created on each request.

With the current version, there is one pool per request. Then if you have 160 simultaneous requests, there are 160 pools, then more connection than a typical Mariadb server can handle. The correct behaviour is 1 pool for the whole Web-framework, and it is the behaviour I submit with my proposal.

The one pool per request break easily if the Dream server is loaded. I can’t expect it to be the expected behaviour. If it does, it MUST be documented as such and the Dream programmer invited to implement its own pool (quite easy, but must be done).

[Gopiandcode]

If you setup your server as presented in the documentation:

let () =
  Dream.run
  @@ Dream.sql_pool "sqlite3:db.sqlite" (* a single pool_cell ref is allocated here *)
  @@ fun request ->
    Dream.sql request (fun db ->
      (* ... *) |> Dream.html)

Then there should only be one SQL pool for your server, because on the first request the pool will be allocated and subsequent requests will use the existing pool:

let sql_pool ?size uri =
    let pool_cell = ref None in
    fun inner_handler request ->

  begin match !pool_cell with
  | Some pool ->
     (* use existing pool *)

  | None -> 
    (* .. *)
    (* create a new pool *)
    pool_cell := Some pool
    (* .. *)

Your change would only change the semantics if you were calling Dream.sql_pool inside each request which I think is not the intended usage.

[F-Loyer]

Yes, I had used a complicated instanciation which takes a token from the request… with a simplier approach, it works.

[aantron]

I understand that this issue is resolved. @paurkedal and @Gopiandcode have the right solution. I wanted to add something to the docs to make it more clear that this is the intended usage, but I don't immediately see how to do it without making the docs overly verbose.