Skip to content

[release/6.0] Fix missing metadata for ThreadPool Event #68876

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
Merged
carlossanlop merged 1 commit into from
May 14, 2022
Merged

[release/6.0] Fix missing metadata for ThreadPool Event #68876

carlossanlop merged 1 commit into from
May 14, 2022

Conversation

@josalem
Copy link
Contributor

@josalem josalem commented May 4, 2022 •
edited
Loading

fixes #65630

Minimal version of #67150

Customer Impact

This is a customer-reported issue.

If a user subscribes an EventListener to threadpool events in .NET 6 on Windows, it may cause a NullReferenceException when the EventListener is disposed.

Root cause: There is a missing ThreadPool event method on NativeRuntimeEventSource for overlapped IO. This event was never implemented, so EventSource never generates the metadata for it. When the EventListener receives this event and tries to look up the metadata (stored in an array of structs), it encounters null values unexpectedly. This causes the Task to fail. This Task is awaited in the Dispose method and renders the exception then.

See #65630 for more details

Testing

This patch includes a test and is currently passing in main. This test covers this issue as well as the overall functionality of NativeRuntimeEventSource.

Risk

Low risk.

In order to actualize the NRE from the issue, a user must do the following:

  1. Be on Windows.
  2. Create an implementation of EventListener
  3. Enable ThreadPool events
  4. Trigger OverlappedIO <-- causes the NRE on a background task and stops all events from being read in the EventListener
  5. Dispose the EventListener <-- actualizes the NRE by awaiting the background task

@josalem josalem self-assigned this May 4, 2022
@ghost
Copy link

ghost commented May 4, 2022

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

fixes #65630

Minimal version of #67150

Customer Impact

If a user subscribes an EventListener to threadpool events in .NET 6 on Windows, it may cause a NullReferenceException when the EventListener is disposed.

Root cause: There is a missing ThreadPool event method on NativeRuntimeEventSource for overlapped IO. This event was never implemented, so EventSource never generates the metadata for it. When the EventListener receives this event and tries to look up the metadata, it encounters null values unexpectedly. This causes the Task to fail. This Task is awaited in the Dispose method and renders the exception then.

See #65630 for more details

Testing

This patch includes a test and is currently passing in main.

Risk

Low risk.

Author: josalem
Assignees: josalem
Labels:

area-System.Threading
Milestone: -

@runfoapp runfoapp bot mentioned this pull request May 5, 2022
Closed
@josalem
Copy link
Contributor Author

josalem commented May 10, 2022

/azp run runtime

@azure-pipelines
Copy link

azure-pipelines bot commented May 10, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlop
Copy link
Contributor

carlossanlop commented May 11, 2022

@josalem this is still marked as draft. Please add the servicing consider label when ready, so it goes through Tactics. Code complete due date for servicing is May 16th.

@josalem
Copy link
Contributor Author

josalem commented May 11, 2022

@josalem this is still marked as draft. Please add the servicing consider label when ready, so it goes through Tactics. Code complete due date for servicing is May 16th.

I was hoping to sort out the CI failures first, but none seem related to the changes. I'll flip the switch first and triage the failures while it's reviewed.

@josalem josalem marked this pull request as ready for review May 11, 2022 18:43
@josalem josalem added the Servicing-consider Issue for next servicing release review label May 11, 2022
@josalem josalem requested review from jeffschwMSFT, kouvel and noahfalk and removed request for noahfalk May 11, 2022 18:43
@josalem
Copy link
Contributor Author

josalem commented May 11, 2022

Triaging CI issues:

  • CoreCLR Win-x86: System.Diagnostics.Eventing.Reader.EventLogException : The system cannot find message text for message number 0x%1 in the message file for %2. failure appears to be intermittent across all pipelines from before this change.
  • macOS failures: all System.IO.Compression.Tests.ZipFile_Unix.UnixCreateSetsPermissionsInExternalAttributes fail on OSX #68293
  • Mono Android: Appears to be some form of deployment error onto the test device from ADB.
  • There seems to be inconsistency between the GH checks and what is failed in AzDO.

@JulieLeeMSFT
Copy link
Member

JulieLeeMSFT commented May 11, 2022

@josalem this is still marked as draft. Please add the servicing consider label when ready, so it goes through Tactics. Code complete due date for servicing is May 16th.

@carlossanlop I will take care of adding servicing-consider label for runtime team PRs.

@JulieLeeMSFT JulieLeeMSFT removed the Servicing-consider Issue for next servicing release review label May 11, 2022
@JulieLeeMSFT
Copy link
Member

JulieLeeMSFT commented May 11, 2022

I will add servicing-consider label once it receives code review.

@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.x milestone May 11, 2022
@JulieLeeMSFT JulieLeeMSFT added the Servicing-consider Issue for next servicing release review label May 11, 2022
jeffschwMSFT
jeffschwMSFT approved these changes May 11, 2022
View reviewed changes
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. Can you highlight that this issue was customer reported and that we had a test hole (now filled)? We will take for consideration in 6.0.x

@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 12, 2022
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.6 May 12, 2022
@carlossanlop carlossanlop merged commit 21d3425 into dotnet:release/6.0 May 14, 2022
@tommcdon tommcdon mentioned this pull request May 17, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jun 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jeffschwMSFT jeffschwMSFT jeffschwMSFT approved these changes

@noahfalk noahfalk Awaiting requested review from noahfalk

+1 more reviewer

@kouvel kouvel kouvel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@josalem josalem

Labels

area-System.Threading Servicing-approved Approved for servicing release

Projects

None yet

Milestone

6.0.6

Development

Successfully merging this pull request may close these issues.

6 participants

@josalem @carlossanlop @JulieLeeMSFT @jeffschwMSFT @kouvel @rbhanda