wakeup-cleanup: only allow one wakeup calblack per I/O operation

[seliopou]

Cleanup wakeup callback bookkeeping that got a bit hairy as a result of #158. Body only allows a single wakeup callback to be registered with it, so just propagate that constraint up to the server connection layer.

[Lupus]

@seliopou @dpatti This PR seems to introduce a regression, starting from it's merge point and up to latest master (d22250e as of now) tests of our in-house framework build on top of httpaf hang. I've looked at test that hangs, it does not seem to do anything complicated, creates http server with handler, serving json payload, and a client which requests it (both built on httpaf).

[seliopou]

Could you send a PR with a rest case that exhibits the bug? Given the test framework it should be possible to exactly replicate the inputs and outputs by looking at an strace of the client and/or server, but let me know if you need any help with that.

…

-Spiros E. i sent this from my Phone.

On Apr 2, 2020, at 3:43 PM, Konstantin A. Olkhovskiy ***@***.***> wrote:  @seliopou @dpatti This PR seems to introduce a regression, starting from it's merge point and up to latest master (d22250e as of now) tests of our in-house framework build on top of httpaf hang. I've looked at test that hangs, it does not seem to do anything complicated, creates http server with handler, serving json payload, and a client which requests it (both built on httpaf). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[seliopou]

Doug and I took a look at this and think we know the cause of it.

[dpatti]

Some context on what was going on here for the record: one subtle change introduced in this PR is that we always transfer the writer callback to Reqd , even if it's empty. The specific situation that could occur is:

1. You have not started the write loop and thus haven't yielded for the first time
2. The read loop finishes and the default (noop) thunk was transferred to the Reqd
3. The write loop starts and immediately yields, also transferring to the Reqd

That third step caused an exception because there was already a callback registered. I'm curious @Lupus if your httpaf runtime was actually raising an exception inside the write loop but it was somehow being swallowed, so it appeared as though the request was just hanging.

I also believe that if the write loop is started before the read loop, this would not happen because in step two we'd be transferring a real callback and wouldn't have an issue of callbacks colliding. I think this is why we never noticed this in practice.

The fix of introducing the optional thunk was both a nice refactor to pull a common pattern out of three modules, and it made it more clear that we shouldn't pass a none thunk to another module (though actually, now that the concept of none is centralized, even if we mistakenly transferred it to Reqd, it wouldn't have caused an issue in this situation).

[Lupus]

Thanks for the details @dpatti !

We've been tweaking exceptions in our Lwt adapter fork, I don't recall the details but we recently tried pinning httpaf to latest master in one of our services (but before this regression was fixed) and got this error:

Request reply exception: (Failure
  "httpaf.Reqd.respond_with_streaming: invalid state, currently handling error")

Looks like it might be related.