-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Move away from exposing internal EventPipe wait handle to EventPipeEventDispatcher. #68320
Conversation
CC @josalem |
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.
LGTM 👍 . There are going to be code conflicts once the other PR merges, but they should be easy to clean up. Just fighting some new CodeAnalysis rules and that should be ready for merging.
9cbca09
to
a3244d9
Compare
Test needs to be disabled on browser since EventPipe is not running on that platform. |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
Failures looks infra related, test suite pass but seems to be permission related errors on phyton script:
Same error occurs on other PR's. |
…entDispatcher. Fix for issue, dotnet#68032. EventPipe exposed and internal wait handle to EventPipeEventDispatcher. The wait handle is not a managed WaitableObject, but maps to runtime specific event types: CLREventStatic * on CoreCLR mono_w32event_create on Mono Overtime it appears that the underlying WaitHandle changed on none Windows platforms, so calls like: EventWaitHandle.Set Assumes that IntPtr used is a WaitableObject and on Mono this will cause a crash, but on CoreCLR, it will just be casted over to a WaitableObject and use it, so based on memory layout this could cause issue, but looking at the implementation, it appears it will always assume EventWaitHandle is signaled so won't touch any of the memory (but not signal the event). There have been an ambition in DispatchEventsToEventListeners to move this abstraction down into native code, get away from using a handle returned from EventPipeInternal.GetWaitHandle as a EventWaitHandle. This PR eliminate EventPipeInternal.GetWaitHandle and adds a SignalSession and WaitForSessionSignal icall's (Mono), qcall's (CoreCLR) and fix the issue caused by exposing the handle to managed code.
f2bf482
to
ad3671f
Compare
Will investigate CoreCLR failing tests. |
All pass, so could be an intermediate failure, will re-run runtime tests on more time. |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
All pass again, looks good. |
Fix for issue, #68032.
EventPipe exposed and internal wait handle to EventPipeEventDispatcher. The wait handle is not a managed WaitableObject, but maps to runtime specific event types:
CLREventStatic * on CoreCLR
mono_w32event_create on Mono
Overtime it appears that the underlying WaitHandle changed on none Windows platforms, so calls like:
EventWaitHandle.Set
Assumes that IntPtr used is a WaitableObject and on Mono this will cause a crash, but on CoreCLR, it will just be casted over to a WaitableObject and use it, so based on memory layout this could cause issue, but looking at the implementation, it appears it will always assume EventWaitHandle is signaled so won't touch any of the memory (but not signal the event).
There have been an ambition in DispatchEventsToEventListeners to move this abstraction down into native code, get away from using a handle returned from EventPipeInternal.GetWaitHandle as a EventWaitHandle. This PR eliminate EventPipeInternal.GetWaitHandle and adds a SignalSession and WaitForSessionSignal icall's (Mono), qcall's (CoreCLR) and fix the issue caused by exposing the handle to managed code.