allthingssecurity commented on PR #28537: URL: https://github.com/apache/camel/pull/28537#issuecomment-6439884867
@gnodet thanks, good catch. Fixed in 7a0d5f6f2028 and 7b69c175e5bd (two new commits on top). **Double completion:** `discard()` now returns the result of `finish()`, and the catch block only sets the exception and calls `callback.done(true)` when it wins. One difference from your snippet: when `discard()` loses, `process()` returns `false`, not `true`. The stop won the CAS and completed the exchange through `super.onError(...)`, which calls `callback.done(false)`, and the `AsyncProcessor` contract is that `process()` returns the same `doneSync` it passed to the callback. Returning `true` there would let the caller continue the exchange on this thread as well as in the callback. **Related gap:** in libthrift 0.25.0 `TAsyncClientManager.stop()` only asks the selector thread to finish, and `call()` checks `selectThread.isAlive()`. So a call registered after `doStop` failed the pending calls can still be accepted by the ending selector thread, which never completes it. In 7a0d5f6f2028 `process()` failed such a call when the producer was stopping or stopped. That check misses the call when the producer is stopped and started again before `process()` gets to it (already while the restart is `STARTING`), so 7b69c175e5bd checks the client instead: `process()` reads the client after registering the call and, after making the call, fails it (`pending.stopped()`, so the CAS still decides who completes it) when it is no longer the producer's client. `doStop` clears the client of the asynchronous producer before it fails the pending calls and `doStart` creates a new one, so either the stop sees the call or `process()` does, also across a restart. A call to a stopped producer is failed with the same `CamelExchangeException`. `thriftClient` is also `volatile` now, since each start replaces it and `process()` reads it. **Tests:** 5 new tests in `ThriftProducerAsyncInFlightStopTest`. They run on one thread, with no sleeps or latches, and put a stub client into the producer by reflection (no hook in the producer). - `aCallThatFailsToStartWhileTheProducerStopsIsCompletedOnce`: the stub's `ping` stops the producer (which fails the pending call) and then throws `TException`. It expects exactly one `done(false)`, `process()` returning `false`, and the stop's `CamelExchangeException` on the exchange. Before the fix: `expected: <[false]> but was: <[false, true]>`. - `aCallAcceptedAfterTheProducerFailedItsPendingCallsIsFailed` and `...AlsoWhenTheProducerStartsAgain`: the stub's `ping` makes the stop miss the call (it clears the set of pending calls first, as when the stop runs while the call is being registered), stops the producer, in the second test starts it again, and returns as if the call was accepted. Both expect exactly one `done(false)`. With 7a0d5f6f2028 the restart variant fails with `expected: <[false]> but was: <[]>`. - `aCallToAStoppedProducerIsFailed`: a call to a stopped producer completes once with the `CamelExchangeException`. - `aCallThatFailsToStartIsCompletedSynchronously` (control): without a stop, a call that cannot be started completes with `done(true)` and `process()` returns `true`. I also ran a local stress test (not committed): 2000 iterations of start, 3 threads making 3 calls each, and a stop (in a quarter of the iterations stop, start, stop; in some a stop from inside a continuation, also on the selector thread) at a random point, against a server that is silent, closes after reading, or answers. All 18000 exchanges completed exactly once, with the `doneSync` that `process()` returned, no pending call was left, and no selector thread survived. camel-thrift unit tests: 64 run, 0 failures, also with the branch merged with the current main. The formatter and import sort made no changes. I updated the description to match. _Claude Code on behalf of allthingssecurity_ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
