Experiment with IteratingCallback

[gregw]

The previous semantic of onCompleteFailure has been renamed to onFailure(Throwable), which is called immediately (but serialized) on either an abort or a failure. A new onCompleteFailure(Throwable) method has been added that is called only after a failed(throwable) or a abort(Throwable) followed by succeeded() or `failed(Throwable)``

No usage has yet been made of the new onCompleteFailure, but the ICB implementation has been completely replaced by the one developed in #11876

[lorban]

IMHO the onCompleteXyz trio should be improved to leave no trace to confusion in the API. Otherwise LGTM.

[sbordet]

I think this is way too fine-grained.

I would be fine to "delay" the call to onCompleteFailure() until completeness (rather than early failure), and that's it.

The new methods introduced are currently not used, and will be APIs that we need to maintain and carefully document about what can and cannot be done in them, which I think is a big effort not worth it.
Their semantic is delicate and we got it mostly wrong in this PR (e.g. calling release from onFailure()).

So I'd revert to minimal changes, i.e. just "delay" properly the call to onCompleteFailure(), and all the rest should just be correct as it is.

[sbordet]

@gregw I think it is mandatory that "release" operations in case of failure become in fact "remove" operations, when dealing with buffers, because it is always possible that the failure is asynchronous, and the buffer is being utilized by the async operation while the release is trying to put it back into the pool for reuse.
It cannot be reused.

Therefore, rather than splitting the "fail" and "release" operations, the former in onFailure() and the latter in onCompleteFailure() (which I have done as an experiment in in branch experiment/jetty-12.1.x/IteratingCallback-SplitRelease), we should instead do the "fail" and "remove" actions from onFailure().

Which then begs the question if we need to have the additional methods introduced in this PR especially regarding abort/failure.
The reason is that current code overriding onCompleteFailure() would have a different behavior with this PR, possibly breaking users like #12055 showed.

So the idea is to have onCompleteFailure() behave like it always did, but fix the "release" operations in case of failure with "remove" operations, and not add the new methods such as onAborted(), onFailed(), etc.

Or, we bit the bullet, change the behavior in 12.1.x, introduce the new methods, possibly break existing code, but avoid the split of my experiment PR by introducing "remove" semantic to RBB.

Dismissing after additional discussions with @lorban.

[gregw]

@lorban Since #12055 has been closed by @sbordet, can we review this one. The aim here is to provide the option for the new semantic, with better naming, but to not change behaviour. Behavior changes can be done bit by bit in subsequent PRs.

[gregw]

@lorban can you take this one on?

[sbordet]

@gregw a note to self/team about failures in case of non termination.

The practical example would be a write that is TCP congested, and a timeout happening.

onFailure() would be called, but the write won't complete unless TCP congestion resolves, or the endPoint is closed in onFailure() (I assume that closing the endPoint would fail the write, but needs to be tested).

[gregw]

@gregw a note to self/team about failures in case of non termination.

The practical example would be a write that is TCP congested, and a timeout happening.

onFailure() would be called, but the write won't complete unless TCP congestion resolves, or the endPoint is closed in onFailure() (I assume that closing the endPoint would fail the write, but needs to be tested).

@sbordet ... and further to that example, we have two strategies to deal with the scenario:

a) in onFailure we remove the write buffer from the pooling, so that it will not be reused ever.
b) we move releasing the write the write buffer to onCompleteFailure, so it is reused, but only once the congestion is resolved and the write completes (or fails)

The buffer is held in memory in both a) and b), so there is no memory saving in either. In a) we potentially free a slot in the buffer pool making for better pooling for other requests, but only a very marginal gain. So the choice between a) and b) really should be based on code clarity and what other events might be triggered by onFailure and/or onCompleteFailure. I think both are valid, so we should make remove() a first class operation on RBB

[lorban]

FYI HttpClientStreamTest.testInputStreamContentProviderThrowingWhileReading() seems to fail because of a leak introduced in FCGI.

[gregw]

@sbordet are you OK if I dismiss @lorban's review and merge this once I get a build clean of flakes?

On vacation!

[sbordet]

@gregw I'm ok to merge this.

[gregw]

The QoSHandler CI failures appear to be a problem in head, so merging this without a green build.