-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Bugfix InvalidOperationException/IndexOutOfRangeException in HttpListener.EndGetContext #107804
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
Bugfix InvalidOperationException/IndexOutOfRangeException in HttpListener.EndGetContext #107804
Conversation
…ust have exclusive access
@dotnet-policy-service agree [company="Jannesen"] |
@dotnet-policy-service agree company="Jannesen" |
} | ||
lock ((DisconnectResults as ICollection).SyncRoot) | ||
|
||
var disconnectResults = DisconnectResults; |
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.
The local variable isn't helpful as it doesn't contain any common expression.
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.
Definition of DisconnectResults
private Dictionary<ulong, DisconnectAsyncResult> DisconnectResults =>
LazyInitializer.EnsureInitialized(ref _disconnectResults, () => new Dictionary<ulong, DisconnectAsyncResult>());
So every access of DisconnectResults is a call to LazyInitializer.EnsureInitialized.
So using the local variable only one call is necessary.
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.
What do you want to do?
Remove the local variable and directly access DisconnectResults (this will result in more Volatile.Read of _disconnectResults) or do you have another 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.
HttpListener isn't so optimized. I don't think reducing several volatile reads can result in real world difference. Let's focus on fixing only.
Tagging subscribers to this area: @dotnet/ncl |
Thanks for the contribution! I'll take a look at this PR, asap! |
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.
Change LGTM in general, but I also think that if we should consider using ConcurrentDictionary
instead of Dictionary
and locking it with SyncRoot
everywhere. However, that's another PR's concern, IMO.
…ust have exclusive access (dotnet#107804)
…ust have exclusive access (dotnet#107804)
/backport to release/9.0 |
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/12317158203 |
@rokonec an error occurred while backporting to release/9.0, please check the run log for details! Error: @rokonec is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=rokonec |
/backport to release/9.0 |
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/12317257049 |
/backport to release/9.0-staging |
Started backporting to release/9.0-staging: #110695 |
Fixes #107025