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

Move away from exposing internal EventPipe wait handle to EventPipeEventDispatcher. #68320

Merged
merged 3 commits into from
May 7, 2022

Conversation

lateralusX
Copy link
Member

@lateralusX lateralusX commented Apr 21, 2022

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.

@lateralusX
Copy link
Member Author

CC @josalem

Copy link
Contributor

@josalem josalem left a 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.

@lateralusX
Copy link
Member Author

@josalem Rebased after merge of #67150 and re-enable disabled test on Mono.

@lateralusX
Copy link
Member Author

Test needs to be disabled on browser since EventPipe is not running on that platform.
Not sure about failure in llvmaot, but could be due to disabled EventPipe as well, will check.

@josalem
Copy link
Contributor

josalem commented May 2, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lateralusX
Copy link
Member Author

lateralusX commented May 3, 2022

Failures looks infra related, test suite pass but seems to be permission related errors on phyton script:

Traceback (most recent call last):
  File "/root/helix/work/correlation/reporter/run.py", line 13, in <module>
    from test_results_reader import read_results
  File "/root/helix/work/correlation/reporter/test_results_reader/__init__.py", line 3, in <module>
    from helix.public import TestResult, TestResultAttachment
  File "/root/helix/scripts/helix/public/__init__.py", line 5, in <module>
    import helix.event
  File "/root/helix/scripts/helix/event.py", line 7, in <module>
    import helix.logs
  File "/root/helix/scripts/helix/logs.py", line 43, in <module>
    rotating_file_handler = RotatingFileHandler(log_file_path, maxBytes=20000000, backupCount=10)
  File "/usr/lib/python3.7/logging/handlers.py", line 147, in __init__
    BaseRotatingHandler.__init__(self, filename, mode, encoding, delay)
  File "/usr/lib/python3.7/logging/handlers.py", line 54, in __init__
    logging.FileHandler.__init__(self, filename, mode, encoding, delay)
  File "/usr/lib/python3.7/logging/__init__.py", line 1092, in __init__
    StreamHandler.__init__(self, self._open())
  File "/usr/lib/python3.7/logging/__init__.py", line 1121, in _open
    return open(self.baseFilename, self.mode, encoding=self.encoding)
PermissionError: [Errno 13] Permission denied: '/root/helix/logs/run.py.log'

Same error occurs on other PR's.

lateralusX added 3 commits May 4, 2022 10:34
…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.
@lateralusX lateralusX force-pushed the lateralusX/fix-issue-68032 branch from f2bf482 to ad3671f Compare May 4, 2022 08:34
@lateralusX
Copy link
Member Author

lateralusX commented May 6, 2022

Will investigate CoreCLR failing tests.

@lateralusX
Copy link
Member Author

lateralusX commented May 6, 2022

All pass, so could be an intermediate failure, will re-run runtime tests on more time.

@lateralusX
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lateralusX
Copy link
Member Author

All pass again, looks good.

@lateralusX lateralusX merged commit e67e4da into dotnet:main May 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Mono] test failure: SIGABRT in nativeruntimeeventsource.cs
2 participants