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.

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

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

Conversation

josalem
Copy link
Contributor

@josalem josalem commented May 4, 2022

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

@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: -

@josalem
Copy link
Contributor Author

josalem commented May 10, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlop
Copy link
Member

@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 noahfalk, kouvel and jeffschwMSFT 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

@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

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
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
@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.
Labels
area-System.Threading Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants