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

System.Net.Http.WinHttpHandler.StartRequestAsync assertion failed #109799

Merged
merged 3 commits into from
Feb 3, 2025
Merged

System.Net.Http.WinHttpHandler.StartRequestAsync assertion failed #109799

merged 3 commits into from
Feb 3, 2025

Conversation

pedrobsaila
Copy link
Contributor

Fixes #83486

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Nov 13, 2024
This was referenced Nov 14, 2024
Open
Open
Open
@CarnaViire CarnaViire self-assigned this Nov 14, 2024
liveans
liveans previously approved these changes Dec 9, 2024
View reviewed changes
Copy link
Member

@liveans liveans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@karelz karelz added this to the 10.0.0 milestone Dec 9, 2024
@liveans liveans dismissed their stale review December 9, 2024 14:48

Natalia had some concerns, need to deep-dive a bit more

@karelz
Copy link
Member

karelz commented Dec 17, 2024

@pedrobsaila FYI - People disappeared for the holidays, so we will not be able to get back to it until January. Hopefully it is alright. Sorry for the inconvenience!

@pedrobsaila
Copy link
Contributor Author

no worries @karelz happy holidays to all the team

@CarnaViire
Copy link
Member

Sorry for the delay. Now I'm back from vacation, so I will be on it shortly.

@ManickaP
Copy link
Member

There's also another place with DisposeAndClearHandle:

runtime/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs

Line 1036 in 99b373a

SafeWinHttpHandle.DisposeAndClearHandle(ref connectHandle);

Could you also replace it with an ordinary dispose - it's on a local variable so not nulling won't affect anything. So that we are consistent in how we dispose handles.

Lastly, can we use Interlocked.CompareExchange in Dispose:

runtime/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs

Lines 542 to 544 in 99b373a

if (!_disposed)
{
_disposed = true;

to be 100% sure that we don't call dispose twice on the session handle in some obscure race condition (which could mess up reference counting for the handle).

Otherwise, LGTM. And apologies for taking this long with the review. If you've already forgotten about this PR, I can take over and work in feedback.

@build-analysis build-analysis bot mentioned this pull request Feb 2, 2025
Open
ManickaP
ManickaP approved these changes Feb 3, 2025
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you very much for the contribution and patience.

@ManickaP ManickaP merged commit ad382ca into dotnet:main Feb 3, 2025
83 of 86 checks passed
@pedrobsaila pedrobsaila deleted the 83486 branch February 3, 2025 10:04
grendello added a commit to grendello/runtime that referenced this pull request Feb 3, 2025
@grendello
Merge branch 'main' into dev/grendel/android-clr-host-local
28eb88e 
* main:
  System.Net.Http.WinHttpHandler.StartRequestAsync assertion failed (dotnet#109799)
  Keep test PDB in helix payload for native AOT (dotnet#111949)
  Build the RID-specific System.IO.Ports packages in the VMR (dotnet#112054)
  Always inline number conversions (dotnet#112061)
  Use Contains{Any} in Regex source generator (dotnet#112065)
  Update dependencies from https://github.com/dotnet/arcade build 20250130.5 (dotnet#112013)
  JIT: Transform single-reg args to FIELD_LIST in physical promotion (dotnet#111590)
  Ensure that math calls into the CRT are tracked as needing vzeroupper (dotnet#112011)
  Use double.ConvertToIntegerNative where safe to do in System.Random (dotnet#112046)
  JIT: Compute `fgCalledCount` after synthesis (dotnet#112041)
  Simplify boolean logic in `TimeZoneInfo` (dotnet#112062)
  JIT: Update type when return temp is freshly created (dotnet#111948)
  Remove unused build controls and simplify DotNetBuild.props (dotnet#111986)
  Fix case-insensitive JSON deserialization of enum member names (dotnet#112028)
  WasmAppBuilder: Remove double computation of a value (dotnet#112047)
  Disable LTCG for brotli and zlibng. (dotnet#111805)
  JIT: Improve x86 unsigned to floating cast codegen (dotnet#111595)
  simplify x86 special intrinsic imports (dotnet#111836)
  JIT: Try to retain entry weight during profile synthesis (dotnet#111971)
  Fix explicit offset of ByRefLike fields. (dotnet#111584)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ManickaP ManickaP ManickaP approved these changes

@liveans liveans liveans left review comments

Assignees

@CarnaViire CarnaViire

Labels
area-System.Net.Http community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.

System.Net.Http.WinHttpHandler.StartRequestAsync assertion failed
5 participants
@pedrobsaila @karelz @CarnaViire @ManickaP @liveans