-
Notifications
You must be signed in to change notification settings - Fork 526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
8d8197d
to
d3e3ede
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AndroidClientHandler
will 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
AndroidClientHandler
object itself is already disposed (since thecancellation request has been applied) and throw the
ObjectDisposedException
instead 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
GC
class)AndroidClientHandler
instances or in other situationswhich 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.