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

Fix EventPipe.Enable() to be aware of multiple EventPipe sessions #34984

Closed
wants to merge 6 commits into from

Conversation

sywhang
Copy link
Contributor

@sywhang sywhang commented Apr 15, 2020

This fixes #34839

@sywhang sywhang added the NO-REVIEW Experimental/testing PR, do NOT review it label Apr 15, 2020
@sywhang sywhang requested review from noahfalk and josalem April 17, 2020 23:38
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.

I think we should add some sort of mechanism to prevent multiple callers from disabling each other's sessions (see comment). We don't expect there to be external users, but nothing stops someone from getting at this API via reflection. Chatted offline, this level of control is unnecessary here 😄

@sywhang sywhang requested a review from josalem April 18, 2020 00:09
@josalem josalem dismissed their stale review April 18, 2020 00:12

Chatted offline and the concerns weren't applicable to this code's use case

@sywhang sywhang removed the NO-REVIEW Experimental/testing PR, do NOT review it label Apr 18, 2020
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.

small nit on the test, but otherwise looks good.

Comment on lines 215 to 216
// Sleep for 5 second to let some events to be written
Thread.Sleep(1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment doesn't match code. Also, does the sleep need to be 1s or could it be something shorter, like 500ms?

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

#34839 suggests that this API was intended to be thread-safe and capable of supporting multiple sessions. I doubt either of those traits were in the original design criteria and it makes me question if the design used here where we retroactively change the API semantics is the best overall choice for how to solve the problem. I'm not sure if there was already discussion about how to solve this but I couldn't find anything written in the issue or the PR here that answered some of the questions that came to mind:

  • What value does Service Profiler get from multiple concurrent sessions?
  • Does the API need to be safe for free-threaded access or can Service Profiler do their own synchronization?
  • Is there any existing functionality that could be used to achieve Service Profiler's goal (such as NetCore.Client APIs)? If the existing tech doesn't work well for the scenario, why not?
  • Any other design options we've looked at and what was appealing about doing it this way vs. alternatives?

@stephentoub
Copy link
Member

@sywhang, are you still working on this? Thanks.

@sywhang sywhang closed this Oct 5, 2020
@sywhang sywhang deleted the dev/suwhang/34839 branch October 5, 2020 14:51
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
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.

Enabling EventPipe via the managed EventPipe class isn't thread-safe
5 participants