Netty provider doesn't notify handler that a chunk is the last one

[slandelle]

Consequence: one cannot expect a HttpResponseBodyPart.isLast returning true in onBodyPartReceived.

The thing is that Netty notifies the end of the response stream with a special fake empty chunk. From my perspective, passing this fake chunk to the handler is just a waste (more work for computing response body) and you get notified in onCompleted anyway.

We have to come up with a decision here. Fix for 1.7.x? Drop isLast in 2.0?

Note that if we decide to fix, Netty and Grizzly providers won't notify the same number of chunks, so we'll have to adapt the tests.

@jfarcan @rlubke WDYT?

[rlubke]

I'd be +1 with dropping isLast().

[slandelle]

For the record, the way to change the behavior and propagate the empty last chunk is to change chunk.isLast() || updateBodyAndInterrupt into chunk.isLast() | updateBodyAndInterrupt.

[slandelle]

OK, I think I get it. isLast is intended to be use to execute bodyPart.markUnderlyingConnectionAsToBeClosed().

If we were to drop isLast, I think we'd have to drop this one too. I can't tell if people use it or not (forcing connection to be closed regardless keep-alive set up and response header).

Here's the current situation:

• AsyncStreamHandlerTest.closeConnectionTest is wrong and doesn't test anything
• Netty issues a last empty chunk for notifying end of response, but the provider doesn't notify the handler's onBodyPart (because it's empty)
• Making Netty provider notify of this empty chunk would require adapting some tests because it would notify of one chunk more than the Grizzly one.

@jfarcand @rlubke Thoughts?

[jfarcand]

@slandelle I agree pushing the empty chunk is a good fix, but we will need to update the documentation to reflect the behavior change. For Grizzly, we could either push an empty chunk or document the behavior difference.

[rlubke]

Sorry for the delay - will be taking a closer look at the Grizzly side of this this week.

[slandelle]

More than an issue, I think we're just facing a situation where Netty and Grizzly behaviors don't match. I'd be in favor of Having Netty push the last chunk, document the behavior difference and adapt the tests on the provider.

[slandelle]

Change broke build on master, reverting.

[slandelle]

I have a fix in the fix428 branch.

With this, users have now to be aware that they can be notified of empty chunks. Note that I had to change ResponseBuilder so that it doesn't accumulate empty parts.

@jfarcand Are you fine with this?