On Tue, Apr 9, 2019 at 10:43 AM Mark Thomas <[email protected]> wrote:
> On 09/04/2019 08:50, Rémy Maucherat wrote:
>
> <snip/>
>
> > Thanks for the fix. I can indeed see that HttpParser.onHeadersComplete
> has:
> > output.headersEnd(streamId); // <- dispatch is done here
> >
> > if (headersEndStream) {
> > output.receivedEndOfStream(streamId);
> > headersEndStream = false;
> > }
> >
> > I guess it's possible to rely on syncing but normally thread dispatching
> > should not occur until after the state is properly set, it's simply
> safer.
>
> I did think about swapping the order of those statements. When I tested
> it I saw one test failure with trailer headers (I didn't investigate
> further) so I went for a different solution.
>
There is a failure indeed, but it's a logging issue only it seems (the
callback log the events in order, so it doesn't match the comparison string
anymore).
>
> It may be the swapping the order is safe but that would need more
> investigation. Also, triggering EOS before end of headers just seems
> wrong to me.
>
I agree it seems wrong. There's another occurrence of the behavior though,
in Http2Parser.readDataFrame, where it does:
if (endOfStream) {
output.receivedEndOfStream(streamId);
}
output.endRequestBodyFrame(streamId);
I will try to investigate, but only after the next build, it's clearly not
worth breaking something.
Rémy
