aantron/dream

Calling `Dream.body` a second time never ends in dev package

Closed this issue · 6 comments

When trying to use the most recent work on dream, it appeared that my previously working code stopped running properly with an infinite loop.
After searching for a bit, I fount out that it were the calls to Dream.body that were causing the problems.

I was therefore able to reproduce the problem with this code:

let hello request =
  let%lwt _body = Dream.body request in
  let%lwt _body = Dream.body request in
  Dream.html ("Good morning world!")

let dream =
  Dream.logger
  @@ Dream.router
  [ Dream.get "/" hello ]
  @@ Dream.not_found

let () =
  Dream.run ~port:8000 dream

Indeed, the second Dream.body should be broken for now. I recommend using 5578b90 or an earlier commit. I'm currently changing the "core" of Dream so that it plugs very nicely with Dream's WIP client library (also using the same core). It's mostly simplification, but there are also some rearrangements, so master is somewhat unstable for now.

A related question: in your usage, is it a big benefit to be able to call Dream.body twice and get the same result? The reason I ask is that that same result is also available in the string promise that Dream.body returns. The body can be accessed from that promise multiple times:

let body_promise = Dream.body request in
let%lwt _body = body_promise in
let%lwt _body = body_promise in

However, to do that across a code base, you'd of course need to pass body_promise around, which might be more annoying and more things to keep track of than just being able to pass the original request around. What do you think about that?

One of the reasons I left double Dream.body broken for now is because that functionality technically is redundant with being able to just read the promise twice (and I simplified away some of the code that used to retain the body), but I'd be happy to restore this feature for the convenience of it.

Thank you for your answer, in my case, i was using Dream.body in a middleware (checking the integrity of the body compared with a signature stocked in the headers), and then again in the regular endpoint. This is how I ended up having 2 calls to the Dream.body function.

As a matter of fact, it is not necessary that a second call to Dream.body returns the same content, but simply being aware of it as a user would help avoiding this problem I guess.

And about passing the promise around, I suppose a self made function like this could work:

let body =
  Dream.new_local ~name:"body" ~show_value:(fun s -> s) ()

let body request =
  match Dream.local body request with
  | Some body -> Lwt.return (request, body)
  | None ->
    let%lwt body' = Dream.body request in
    Lwt.return (Dream.with_local body body' request, body')

And then use it in my middlewares and endpoints without having to worry about Dream.body being called twice.

I suppose however, that it could be more of a problem if someone were to use a middleware coded in another library and which would use the Dream.body without being aware that saving the body promise would be necessary.

Nice. I was also leaning towards using Dream.local to get Dream.body to behave as before.

Note, however, that you should store the body promise (not the body string) in the variable, because there is a race condition otherwise — if another "thread" of code tries to call body above, after the first "thread" has gone into the None case, but before let%lwt body' resolves, it will also start reading the body, instead of using the same promise.

Most likely, this is what Dream will do internally once I sort out the most immediate issues.

Noted thank you very much

I've implemented body promise caching along similar lines to your suggestion and the following discussion:

dream/src/pure/message.ml

Lines 279 to 285 in 3acb3cc

let body message =
match field message body_field with
| Some body_promise -> body_promise
| None ->
let body_promise = Stream.read_until_close message.server_stream in
set_field message body_field body_promise;
body_promise

Dream master also should now be stable. It has had some major changes, such as switching to mutable requests and consequently adopting a t-first style of function signature. I've completely subsumed WebSockets into the new streams, and they no longer have their own little API. I renamed "locals" to "fields" (because globals have been deleted completely). Many functions have been renamed. I haven't yet caught up the posted docs or example READMEs, however all the examples now build against the latest code and I tried to add as many helpful deprecation messages as possible to help convert code. If you want to use master immediately, I recommend referring to dream.mli directly for now.

Thank you a lot for your modifications, but I'll wait for a bit for now, I am working with dream-mirage which as fallen a bit behind with all those recent modifications, I have corrected some of them but I wont have the time to do a pr right now.
Until then I'll stick to my working version and switch later to your updated code.
Thank you a lot for the hard work