-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Fix null ref in UrlGroup.Dispose due to logger not set #26983
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
Conversation
@NGloreous Can you please sign the CLA? |
And re-target to the master branch please? I don't expect this to get approved for 5.0 at this point. We might be able to backport it for servicing later. |
2fb1311
to
da6155e
Compare
@shirhatti CLA signed. @Tratcher updated to point to master. I wasn't quite sure what the process was to get this patched for 5. |
@@ -326,6 +326,10 @@ private void SetFatalResponse(int status) | |||
|
|||
internal unsafe void Delegate(DelegationRule destination) | |||
{ | |||
if (destination == null) |
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.
Nullability should do a better job of validating this. Could we remove this check given that it's a non-public API?
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.
@pranavkm HttpSys.FeatureContext.DelegateRequest which is public calls this method directly. I could move this check over there but figured it made more sense here since this is where it's used.
I don't quite get your point about nullability.
It's too late to get this patched for 5. Given that there's an obvious workaround it won't meet the bar |
@shirhatti what's the work around? If there are any issues in Dispose they will result in a null ref and no ability to have insight into the actual error. |
@NGloreous Can you rebase to fix the merge conflicts? |
da6155e
to
3ac734b
Compare
@shirhatti done. |
Hello @Tratcher! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Summary of the changes (Less than 80 chars)