[AndroidClientHandler] Observe cancellation requests more #4589
Conversation
|
This didn't fix it for me. See details: #4550 (comment) |
There was a problem hiding this comment.
Draft release notes
This looks like a nice user-facing behavior improvement. When you get a chance, you can add a draft release note for it to Documentation/release-notes/4589.md as part of the PR. It looks like the note can probably be a short sentence or two, maybe roughly like:
#### Application behavior on device and emulator
* [GitHub PR 4582](https://github.com/xamarin/xamarin-android/pull/4589):
`ObjectDisposedException` could in theory be thrown during cancellation of
`AndroidClientHandler` requests, depending on the particular timing of
cancellation and object disposal.
The note can maybe include a sentence like "The time window where this can happen is now even narrower," if that's accurate.
|
I'm not sure this actually makes it narrower. During GC I consistently found this change had no effect what so ever, and I was only able to reproduce this issue during GC. |
grendello
force-pushed
the
tune-cancellation
branch
from
697faa6
to
31461fd
Compare
Context: dotnet#4550 It is possible that, in certain situations, a cancellation request arriving from code external to `AndroidClientHandler` will not be satisfied while starting the task to acquire connection status, because the cancellation request appears after handler already established connection to the remote host but before the request status is obtained. Failure to check that the cancellation was requested may result in the status retrieval task starting only to run into the issue that the `AndroidClientHandler` object itself is already disposed (since the cancellation request has been applied) and throw the `ObjectDisposedException` instead of cancelling the status request quietly. Note that this is likely happen only in situations when the application code aggressively cancels and collects (via calls to the `GC` class) `AndroidClientHandler` instances or in other situations which induce potential for timing issues in the TPL or `async/await` state machine, thus leading to possible race conditions there. This commit passes cancellation token to the task which runs the status request, thus preventing it from starting in the situation described above. The possibility of a race condition still remains, but is seriously reduced.
grendello
force-pushed
the
tune-cancellation
branch
from
8d8197d
to
d3e3ede
Compare
There was a problem hiding this comment.
Release note approved. Thanks!
If it turns out in the end that this PR is determined not to be relevant for apps in practice, no worries if is closed rather than merged. If it is merged, then I'll be careful not to change the release note in any way that would imply that a specific known test case is resolved by the change.
One small question, partly of curiosity, is whether this change could be considered an improvement in following Task idioms even if it doesn't correspond to a known test case.
I will do so. |
…4589) Context: #4550 It is possible that, in certain situations, a cancellation request arriving from code external to `AndroidClientHandler` will not be satisfied while starting the task to acquire connection status, because the cancellation request appears after handler already established connection to the remote host but before the request status is obtained. Failure to check that the cancellation was requested may result in the status retrieval task starting only to run into the issue that the `AndroidClientHandler` object itself is already disposed (since the cancellation request has been applied) and throw an `ObjectDisposedException` instead of cancelling the status request quietly. Note that this is likely happen only in situations when the application code aggressively cancels and collects `AndroidClientHandler` instances (e.g. via calls to `GC.Collect()`) or in other situations which induce potential for timing issues in the TPL or `async/await` state machine, thus leading to possible race conditions there. Pass the cancellation token to the task which runs the status request, thus preventing it from starting in the situation described above. The possibility of a race condition still remains, but is reduced.
Context: #4550
It is possible that, in certain situations, a cancellation request
arriving from code external to
AndroidClientHandlerwill not besatisfied while starting the task to acquire connection status, because
the cancellation request appears after handler already established
connection to the remote host but before the request status is obtained.
Failure to check that the cancellation was requested may result in the
status retrieval task starting only to run into the issue that the
AndroidClientHandlerobject itself is already disposed (since thecancellation request has been applied) and throw the
ObjectDisposedExceptioninstead of cancelling the status requestquietly. Note that this is likely happen only in situations when the
application code aggressively cancels and collects (via calls to the
GCclass)AndroidClientHandlerinstances or in other situationswhich induce potential for timing issues in the TPL or
async/awaitstate machine, thus leading to possible race conditions there.
This commit passes cancellation token to the task which runs the status
request, thus preventing it from starting in the situation described
above. The possibility of a race condition still remains, but is
seriously reduced.