[#2198] Fix pending sends count on rolled back send

[mattrpav]

This was uncovered while looking into a different issue.

The problem here is that pending sends counter (internal, not exposed) does not decrement on a rolled back producer transaction.

[cshannon]

@mattrpav - I have asked this several times before, can you please start adding actual information to your PRs and issues?

You added a bunch of info to the test itself which explains the problem, some of that needs to be in the description so people can figure out what the point of the PR is without having to look at the test.

[cshannon]

LGTM, this is a good catch, just a couple minor tweaks that I noticed that could be done

[cshannon]

LGTM to merge, assuming CI looks good. I'm always amazed when stuff like this has been hanging around for such a long time and no one noticed. Another example is #2178 that was broken for a long time

[jbonofre]

I suspect that we could have double-decrement pendingSends in XA. The singlePendingSend() uses <= 1 so negative values can pass the test. Neither fireAfterCommit() and fireAfterRollback() has a guard preventing both from running on the same sync list.
That's OK, and we can always improve in a follow-up PR.

[jbonofre]

nit: instead of reflection, we could have added an accessor with @VisibleForTesting in Queue to avoid coupling the test.

[cshannon]

I almost mentioned this too, and I would have suggested this if we could use protected/package scope as an accessor but it's a different package so it would have to be public which I wasn't sure was a good idea, so i was ok with the reflection in this case

[cshannon]

I just realized that checkUsage() can throw an exception. If that happens, the counter gets incremented but it would never be decremented so I think that needs to be fixed too. We might just need a separate try/catch to wrap pendingSends.increment if we need to increment before checkUsage. I haven't looked at the producerExchange.incrementSend() but i am wondering if that is a problem too with missing decrement on exception.

[cshannon]

I suspect that we could have double-decrement pendingSends in XA. The singlePendingSend() uses <= 1 so negative values can pass the test. Neither fireAfterCommit() and fireAfterRollback() has a guard preventing both from running on the same sync list. That's OK, and we can always improve in a follow-up PR.

With the current design, I am not sure if there is a great way to guarantee we don't miss a decrement or double decrement using a simple counter like it is doing now. The problem is there is so many code paths for the decrement (async etc) and possible error scenarios it would be hard to ensure it's correct.

I think a future PR to refactor to just try and simplify and reduce the number of code paths where increment/decrement happen would help provide better guarantees it's correct. One way to handle the double decrement case would be to to track pendingSend message Ids in a Set instead of using a counter and just remove instead of decrement. However, that doesn't help missing a decrement and would cause a memory leak so not sure that makes sense either.