Cancel write semantic

[gregw]

Added a cancel write/send semantic so that buffers for failed writes need not be removed from the pool

[gregw]

@sbordet can you implement the cancel semantic for http2. @lorban ditto for http3

[gregw]

@sbordet @lorban nudge for actual review

[sbordet]

I am dubious about the H2/h3 implementation; we should talk about it.

[sbordet]

@gregw another issue is the use of CountingCallback: it has the semantic to count for successes, but it fails on the first failure.

So it has not the right semantic for the usage that was done in H2 and H3.

[gregw]

@gregw another issue is the use of CountingCallback: it has the semantic to count for successes, but it fails on the first failure.

So it has not the right semantic for the usage that was done in H2 and H3.

@sbordet See new static List<Callback> from(Callback callback, Throwable cause, int count) method that replaces CountingCallback

[lorban]

I believe the mechanism is sane, but I can't wrap my head around the newly introduced API and how this should all work.

I've attempted to write a test for this functionality (I've pushed it to this PR) and that left me puzzled. We should probably discuss it.

[gregw]

@sbordet @lorban have you looked at this recently? Any updates for h2 or h3?

[sbordet]

I think you want to take them atomically, so here you should use state.get(int[]).
Likewise in the other places.

Actually, my suggestion of AtomicStampedReference was more to handle the Callback and the counter together... Now that the callback is final, I don't think we need AtomicStampedReference anymore, we can split again failure and counter, as now we just use the counter as the state of all the callbacks.

[gregw]

We don't need to take them atomically. If they change between one get and the next, then the CaS will fail anyway. I guess it is only a single volatile lookup, so maybe better... I'll have a look at it

[sbordet]

I would prefer just AtomicInteger for the counter at this point.

[gregw]

I think the stamped reference is better. There are still some races with multiple failures that this solves and gives us more deterministic behaviour.

on vacation

[sbordet]

If this CAS fails, is exception combination above safe?

[gregw]

It is. Failure will only go from null to non-null once and then stay at that value. So we are not trying to hit a moving target. If the cas fails, then the addSuppressed will not add the same exception again, because they will already be associated. But it will be the same target failure, so the previous addSuppressed was the correct thing to do.

Note, it is because of this slight complex race that I had the failure logic after the CaS, but before the the andThen callback. I think it is safe there as well with less thought needed, but you asked me to move it. I think it is probably better where it is now.

[gregw]

@sbordet I'm going to try to get a clean CI build before merging. I think the failures are flakes or CI issues... but will check.

Also, what is the plan for 12.0.x? Can any of this be backported easily? The h2 reset fix?

[sbordet]

Plan for 12.0.x is to rely on releaseAndRemove(), as we don't have the onAborted() and complete event separation semantic in 12.0.x IteratingCallback.

[wendigo]

🎉