SI-8689 Avoid internal error in Promise after sequence of completions #4289
Conversation
|
Retargets #4215 to the 2.10.x branch, from whence it will flow to 2.11.x and 2.12.x. Review by @viktorklang (the author of the patch) and @axel22. I confirmed that the added test cases failed without the change to |
|
Egad! Time to backport the CI scripts from #4268 to 2.10... Looking at it. |
|
If I winged it successfully with #4290, please rebase on top. |
|
👍 |
|
LGTM On 3 February 2015 06:51:47 GMT, Jason Zaugg [email protected] wrote:
Sent from my Android device with K-9 Mail. Please excuse my brevity. |
Calling `completeWith` when the `DefaultPromise` is already completed,
leads to callbacks not being properly executed.
This happened because `Future.InternalCallbackExecutor` extends
`BatchingExecutor`[1] which assumes `unbatchedExecute` to be async,
when in this case it is sync, and if there is an exception thrown
by executing the batch, it creates a new batch with the remaining
items from the current batch and submits that to `unbatchedExecute`
and then rethrows, but if you have a sync `unbatchedExecute`, it will
fail since it is not reentrant, as witnessed by the failed `require`
as reported in this issue.
This commit avoids problem by delegating `completeWith` to
`tryComplete`, which has the effect of using `onComplete` +
`tryComplete` i.s.o. `complete`, which means that when it fails
(because of a benign race condition between completers) it won't
throw an exception.
It has been tested by the minimized reproducer.
[1] Actually, in the 2.10.x branch where this patch is starting out,
"The BatchingExecutor trait had to be inlined into
InternalCallbackExecutor for binary compatibility.". This comment
will be more literally correct in the context of 2.11.x and beyond
|
Rebased on 2.10.x to get PR validation working, no code changes. |
|
The failure can be ignored. Sorry. |
|
/nothingtoseehere |
SI-8689 Avoid internal error in Promise after sequence of completions
Calling
completeWithwhen theDefaultPromiseis already completed,leads to callbacks not being properly executed.
This happened because
Future.InternalCallbackExecutorextendsBatchingExecutor[1] which assumesunbatchedExecuteto be async,when in this case it is sync, and if there is an exception thrown
by executing the batch, it creates a new batch with the remaining
items from the current batch and submits that to
unbatchedExecuteand then rethrows, but if you have a sync
unbatchedExecute, it willfail since it is not reentrant, as witnessed by the failed
requireas reported in this issue.
This commit avoids problem by delegating
completeWithtotryComplete, which has the effect of usingonComplete+tryCompletei.s.o.complete, which means that when it fails(because of a benign race condition between completers) it won't
throw an exception.
It has been tested by the minimized reproducer.
[1] Actually, in the 2.10.x branch where this patch is starting out,
"The BatchingExecutor trait had to be inlined into
InternalCallbackExecutor for binary compatibility.". This comment
will be more literally correct in the context of 2.11.x and beyond