Fix #4461 HttpOutput Aggregation

[gregw]

This is an alternative to #4465 that fixes #4461 and the discovered issue of not aggregating due to an empty at capacity buffer.

• Added tests to check that aggregation continues after first flush of an aggregated buffer (this triggers both IllegalStateException in HttpOutput with Jersey #4461 and the discovered bug of not aggregating because of empty at capacity aggregate buffer).
• Added getAggregateSize method that does a compact to avoid empty at capacity aggregate buffer
• Call onWriteComplete if residue of an overflow aggregation can itself be aggregated.

Signed-off-by: Greg Wilkins [email protected]

Closes #4461

[gregw]

@sbordet @joakime In this PR I have fixed both issues together because I think they are very much related.

I am a bit conflicted about if writes should always do an implicit compact when they have consumed a buffer or not... if so then after writing we would never have a buffer left at p=c,l=c as it would always be compacted to p=0,l=0.

However the javadoc for EndPoint simply says: The header/buffers position is updated to indicate how many bytes have been consumed. So it does not say that we can change the limit to indicate how many bytes are consumed. Thus I think it is correct for HttpOutput to do the compacting (or clearing) in its own reuse of the aggregate buffer, as we should not assume an implicit compact with a write/flush.

So does that mean that the GzipHandler is wrong and it should not do a compact and only modify the position to be equal to the limit (as per #4465) ???

[gregw]

Reading more, the contract for Channel.write(ByteBuffer) says:

Upon return the buffer's position will be equal to p+n; its limit will not have changed.

Thus our normal EndPoints are correct and the clear in GzipHandler is incorrect. I will fix that also in this PR.

[sbordet]

Overall, I liked more the changes in #4465 as they removed code that was not necessary, rather than adding more if/else to cover for more cases.

I would strive for aggregation in just one place, and if a network write must happen, let's write all we have (less latency) rather than writing some and aggregate again.

For Jetty 10 we need HttpOutput.Interceptor be able to do gather writes!

[sbordet]

With this, this test is testing conditions that will never be the ones with the normal Jetty code, rendering this test useless.

[gregw]

Because it is testing aggregation, which is dependent on the actual size of the buffer. As our buffer pools can give back buffers with larger capacities, this test uses a simple buffer pool that gives back a buffer of exact size and thus allows the tests to be determinant about if they do or do not aggregate/commit etc.

[joakime]

The real buffer pool behaviors are one of the reasons the original code failed in a production capacity. I think @sbordet has a point, this should use a real world scenario, not a pie-in-the-sky wishful scenario.

[gregw]

This is not pie in the sky wishful thinking. It is a deterministic test case that actually tests what it thinks it is testing, not some random behaviour that is dependent on a buffer pool beyond the control of the test.

[joakime]

OK, then add an alternate test that tests the opposite case.

A new ByteBufferPool implementation that always returns 4x the size requested.

[gregw]

But that is not what I'm testing. The tests only need to know if a write is bigger, smaller or the same as the buffers. It is relative size not absolute size that is important here. If I had a 4x buffer returning pool, then I'd just have to alter the test case to write 4x as much data in each write to test the scenarios that are needed to test.

The point being that the HttpOutputTest is providing very good (well above average) coverage of HttpOutput. The purpose here is not to test the buffer pools

[gregw]

@sbordet I don't think you have fully understood the problem in your review. #4465 only fixes part of the problem and changes behaviour significantly. I think it has some good simplifications, but they should only be considered for jetty-10.

Your review has not answered the fundamental question that I've asked: where should the compact be done? Options are: a) in the EndPoint; b) after every write, c) immediately before the space is checked?
a) is contrary to the contract for EndPoint and Channel.write(ByteBuffer), I tried b), but it required duplicate code in many places and would be error prone to maintain. This solution goes for c) and the compact is in only one spot, immediately before the space is checked.

Yes the getter has a side effect in one point of view, but you can also argue that a buffer with p==0,l==0 is equivalent to one with p==c,l==c in all ways other than space.

[joakime]

Current state i'm happy with.
Looking forward to the cleanup when it merges to Jetty 10.

[gregw]

I'm now not so sure about a cleanup in 10. When we have a nearly full aggregate buffer that is overflowed by a small write, the options are:

• fill aggregate, flush aggregate and then aggregate remaining: a single optimal write and the current behaviour
• flush aggregate and fill: a non optimal write a less remaining space in aggregate
• flush aggregate and flush buffer: two non optimal writes... unless we can have gather writes all the way down.

So we need gather writes to improve this, which would mean we would need gather generates for HTTP, H2, etc.
It is a lot of work to remove a few lines of code.

I'll open an issue to investigate increased use of gather writes, but it is feeling like a jetty > 10 thing to me.