Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Mono.Android] Fix race condition in AndroidMessageHandler #8753

Merged
merged 2 commits into from
Mar 5, 2024
Merged

[Mono.Android] Fix race condition in AndroidMessageHandler #8753

merged 2 commits into from
Mar 5, 2024

Conversation

simonrozsival
Copy link
Member

Fixes #8740

There is a race condition in AndroidMessageHandler (see #8740 (comment) for more information). The solution is fairly simple: we should determine the decompression strategy only during the setup, when AutomaticDecompression is set. We don't need to do any more work during the request.

To prevent changing the decompression strategy while there are requests mid-flight, the setter will only accept values until the first request is sent. This is exactly what the SocketsHttpHandler does. I didn't want to touch any code unrelated to the race condition in this PR so if we want to prevent other properties from being mutated while HTTP requests are being sent, we should do it in a follow-up PR.

/cc @grendello

@grendello
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tipa
Copy link

tipa commented Feb 23, 2024 •
edited
Loading

Thanks for the quick fix. One small comment/suggestion: Is the _acceptEncoding variable even needed now or wouldn't it be easier to check for _acceptEncoding == null instead?
https://github.com/xamarin/xamarin-android/blob/c2e075912bf0069d8097126c0ad114d58ffc6006/src/Mono.Android/Xamarin.Android.Net/AndroidMessageHandler.cs#L770

@simonrozsival
Copy link
Member Author

@tipa Yes, we could replace decompress_here with _acceptEncoding is not null && _acceptEncoding != IDENTITY_ENCODING 👍

@grendello
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Member

@grendello, @simonrozsival: is there any reasonable way to write a unit test for this?

@simonrozsival
Copy link
Member Author

@jonpryor I haven't come up with a good way to test this. The repro project needs to send several hundreds of requests before it hits the race condition. We could probably use HttpListener and hammer it with requests for several seconds with ~20 threads and make sure all the responses are automatically decompressed.

@grendello
Copy link
Contributor

@jonpryor I don't think there is such a way. What @simonrozsival proposes would sometimes work and would sometimes fail - another test we learn to ignore. No point IMO.

@tipa
Copy link

tipa commented Mar 4, 2024

Any chance this will be merged and released before .NET 9? :)

@grendello grendello merged commit e41a31b into dotnet:main Mar 5, 2024
47 checks passed
grendello added a commit that referenced this pull request Mar 6, 2024
@grendello
Merge branch 'main' into dev/grendel/xxhash3
1a8e48c 
* main:
  [Mono.Android] Fix race condition in AndroidMessageHandler (#8753)
  [ci] Fix SDL Sources Analysis for PRs from forks (#8785)
  [ci] Add 1ESPT override to MSBuild test stages (#8784)
  [ci] Do not use @self annotation for templates (#8783)
  [ci] Migrate to the 1ES template (#8747)
  [Mono.Android] fix trimming warnings, part 2 (#8758)
  [Xamarin.Android.Build.Tasks] set `%(DefineConstantsOnly)` for older API levels (#8777)
  [tests] fix duplicate sources in `NuGet.config` (#8772)
  Bump to xamarin/monodroid@e13723e701 (#8771)
grendello added a commit that referenced this pull request Mar 7, 2024
@grendello
Merge branch 'main' into dev/grendel/extended-timing
873f412 
* main:
  [Mono.Android] Fix race condition in AndroidMessageHandler (#8753)
  [ci] Fix SDL Sources Analysis for PRs from forks (#8785)
  [ci] Add 1ESPT override to MSBuild test stages (#8784)
  [ci] Do not use @self annotation for templates (#8783)
  [ci] Migrate to the 1ES template (#8747)
  [Mono.Android] fix trimming warnings, part 2 (#8758)
grendello added a commit to grendello/xamarin-android that referenced this pull request Mar 8, 2024
@grendello
Merge branch 'main' into blazor-hang
b5fc35f 
* main: (306 commits)
  [templates] Remove redundant "template" from display name. (dotnet#8773)
  Bump to xamarin/Java.Interop/main@a7e09b7 (dotnet#8793)
  [build] Include MIT license in most NuGet packages (dotnet#8787)
  Bump to dotnet/installer@893b762b6e 9.0.100-preview.3.24153.2 (dotnet#8782)
  [docs] update notes about `dotnet-trace` and `dotnet-gcdump` (dotnet#8713)
  [Mono.Android] Fix race condition in AndroidMessageHandler (dotnet#8753)
  [ci] Fix SDL Sources Analysis for PRs from forks (dotnet#8785)
  [ci] Add 1ESPT override to MSBuild test stages (dotnet#8784)
  [ci] Do not use @self annotation for templates (dotnet#8783)
  [ci] Migrate to the 1ES template (dotnet#8747)
  [Mono.Android] fix trimming warnings, part 2 (dotnet#8758)
  [Xamarin.Android.Build.Tasks] set `%(DefineConstantsOnly)` for older API levels (dotnet#8777)
  [tests] fix duplicate sources in `NuGet.config` (dotnet#8772)
  Bump to xamarin/monodroid@e13723e701 (dotnet#8771)
  Bump to xamarin/xamarin-android-tools/main@37d79c9 (dotnet#8752)
  Bump to dotnet/installer@d070660282 9.0.100-preview.3.24126.2 (dotnet#8763)
  Bump to xamarin/java.interop/main@14a9470 (dotnet#8766)
  $(AndroidPackVersionSuffix)=preview.3; net9 is 34.99.0.preview.3 (dotnet#8765)
  [Mono.Android] Do not dispose request content stream in AndroidMessageHandler (dotnet#8764)
  Bump com.android.tools:r8 from 8.2.42 to 8.2.47 (dotnet#8761)
  ...
jonathanpeppers pushed a commit that referenced this pull request Mar 11, 2024
@simonrozsival @jonathanpeppers
[Mono.Android] Fix race condition in AndroidMessageHandler (#8753)
5c04944 
Fixes: #8740

There is a race condition in `AndroidMessageHandler` (see #8740 (comment) 
for more information). 

The solution is fairly simple: we should determine the decompression strategy only during the setup, when `AutomaticDecompression` is set. 
We don't need to do any more work during the request.

To prevent changing the decompression strategy while there are requests mid-flight, the setter will only accept values until the first request 
is sent. This is exactly what the `SocketsHttpHandler` does. I didn't want to touch any code unrelated to the race condition in this PR so if we 
want to prevent other properties from being mutated while HTTP requests are being sent, we should do it in a follow-up PR.
grendello added a commit that referenced this pull request Mar 11, 2024
@grendello
Merge branch 'main' into dev/grendel/blobs-in-lib
fb86fd0 
* main:
  [templates] Remove redundant "template" from display name. (#8773)
  Bump to xamarin/Java.Interop/main@a7e09b7 (#8793)
  [build] Include MIT license in most NuGet packages (#8787)
  Bump to dotnet/installer@893b762b6e 9.0.100-preview.3.24153.2 (#8782)
  [docs] update notes about `dotnet-trace` and `dotnet-gcdump` (#8713)
  [Mono.Android] Fix race condition in AndroidMessageHandler (#8753)
  [ci] Fix SDL Sources Analysis for PRs from forks (#8785)
  [ci] Add 1ESPT override to MSBuild test stages (#8784)
  [ci] Do not use @self annotation for templates (#8783)
  [ci] Migrate to the 1ES template (#8747)
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@grendello grendello grendello approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[AndroidMessageHandler] ReadAsByteArrayAsync occasionally returning incorrect byte array
4 participants
@simonrozsival @grendello @tipa @jonpryor