search

yawaramin / re-web

Experimental web framework for ReasonML & OCaml
https://yawaramin.github.io/re-web/re-web/index.html
MIT License
261 stars 8 forks source link

Lwt.choose over Lwt.pick #32

Open tcoopman opened 4 years ago

tcoopman commented 4 years ago

While I was looking for the error that I had yesterday where some messages didn't arrive, I as that you use Lwt.pick here: https://github.com/yawaramin/re-web/blob/4a8bce346627613694a26f7d38773d251fc106fb/ReWeb/Server.ml#L92

I'm wondering if that shouldn't be Lwt.choose instead.

Lwt.pick wil try to cancel the other promise, so I wondering if that means that some messages might get lost. The behavior of Lwt.choose will not cancel the promise, so I believe that you will get it on the next round.

I don't know Lwt well enough to be sure about this at all, but I'll see if I can maybe create a test case.

tcoopman commented 4 years ago

I cannot get this to fail, but I still feel a bit unsure about it. It's not clear how Lwt_stream.start and cancelled promises work together to me, so that's why I'm unsure about it.

yawaramin commented 4 years ago

I think the message loss thing got fixed upstream, I'll upgrade to that soon (Ulrik and Wojtek refresh their published versions fairly regularly).

Regarding Lwt.choose vs Lwt.pick, based on reading the implementation of the former, it looks like all the promises in the input promise list are allowed to run to completion, then ignored. So the promise that tries to pull a message from the stream, Lwt_stream.get incoming, is already running and will complete (unless it is rejected with an exception), then its result will be thrown away anyway if there was a timeout. So I don't think this will stop the message from getting lost.

yawaramin commented 4 years ago

Thinking about this a bit more, the reason why there would be a timeout when trying to get the next message from the stream, is that the stream is open but it's empty i.e. no message has been pushed to it (yet). So allowing a get promise to run off by itself and then ignoring it, will indeed drop the message that it gets.

Looks like I need to use Lwt.peek instead to return the message within the time limit, and after that's done spin off the promise which actually removes the message from the stream.

tcoopman commented 4 years ago

Ah, yes, that's correct Lwt.choose won't help a lot here. Maybe instead of Lwt.peek you can use Lwt.nchoose and ignore the timeout yourself?

yawaramin commented 4 years ago

Tried Lwt.nchoose, unfortunately it turned out not to work–the promise pulling a message from the incoming stream never resolved, and the server just hung. I'll keep thinking about this.