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

Don't shut down event pipe in DLLs on Windows #91715

Merged
merged 6 commits into from
Sep 14, 2023

Conversation

MichalStrehovsky
Copy link
Member

Works around #89346.

We blocked event pipe completely in shared libraries in #90811 but:

  • Elinor found out the shutdown problem is actually Windows specific, so blocking Linux (our most important target) is unnecessary.
  • We can avoid the hang at the cost of corrupting the ongoing event pipe session by just not doing the shutdown. This can be worked around by simply stopping the event pipe session at dotnet-trace side manually.

I validated the shared library scenario no longer hangs at shutdown with this.

#89346 still tracks if we can do better here.

Cc @dotnet/ilc-contrib

@ghost
Copy link

ghost commented Sep 7, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Works around #89346.

We blocked event pipe completely in shared libraries in #90811 but:

  • Elinor found out the shutdown problem is actually Windows specific, so blocking Linux (our most important target) is unnecessary.
  • We can avoid the hang at the cost of corrupting the ongoing event pipe session by just not doing the shutdown. This can be worked around by simply stopping the event pipe session at dotnet-trace side manually.

I validated the shared library scenario no longer hangs at shutdown with this.

#89346 still tracks if we can do better here.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

Copy link
Member

@LakshanF LakshanF left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@agocke
Copy link
Member

agocke commented Sep 7, 2023

Does it actually work though? Can you load two libraries into the same process, enable event support, and the use dotnet-monitor or similar to retrieve events from both libraries?

I'd rather disable this completely until we're confident the scenario is working as we want it to.

@elinor-fung
Copy link
Member

elinor-fung commented Sep 7, 2023

I'd rather disable this completely until we're confident the scenario is working as we want it to.

What about allowing a bypass? So blocked by default because of the multiple runtime questions, but still let people explicitly enable it as a 'I understand the limitations - I will only have one library/runtime and I want to be able to trace it'. I think that was actually @MichalStrehovsky's original suggestion.

@agocke
Copy link
Member

agocke commented Sep 7, 2023

Yup, bypass sounds great, just like we did with WPF.

…e.Publish.targets

Co-authored-by: Elinor Fung <elfung@microsoft.com>
Copy link
Member Author

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

This is what I had in mind if we really insist on blocking this.

This is different from the WPF block because that one doesn't work at all. This works, for a large majority of users.

@agocke
Copy link
Member

agocke commented Sep 7, 2023

This works, for a large majority of users.

That's what I was asking, though. Does it? Loading two CoreCLR instances in the same process is unsupported. Loading two Native AOT binaries in the same process is fully supported.

If this is fully supported, I would expect it to be fully supported across all our supported configurations.

@MichalStrehovsky
Copy link
Member Author

That's what I was asking, though. Does it? Loading two CoreCLR instances in the same process is unsupported. Loading two Native AOT binaries in the same process is fully supported.

Yes, but so is EventSourceSupport in Native AOT exe files, and CoreCLR hosting APIs. Still if you use all of these in a single process, it will likely hit the same issue as two NativeAOT dlls. I think the right course of action is not in blocking (because we can't block that one), but in defining a failure mode.

@MichalStrehovsky
Copy link
Member Author

@agcoke the last commit downgrades the error to a suppressible warning with a link to the issue. I considered making a fwlink for it but maybe that's an overkill.

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM, minor wording change suggested, and replaced link with a link to our official docs. I figure we can add a line in here to fully explain the situation.

…e.Publish.targets

Co-authored-by: Andy Gocke <andy@commentout.net>
@MichalStrehovsky MichalStrehovsky merged commit 9101b85 into dotnet:main Sep 14, 2023
89 of 108 checks passed
@MichalStrehovsky MichalStrehovsky deleted the allowevt branch September 14, 2023 05:25
@MichalStrehovsky
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6181325936

@ghost ghost locked as resolved and limited conversation to collaborators Oct 14, 2023
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.

5 participants