Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

[Release/3.1] Port profiler APIs to set and retrieve environment variables to 3.1 #27512

Merged
merged 2 commits into from
Oct 30, 2019

Conversation

davmason
Copy link
Member

This ports #27157 to 3.1.

Issue
On linux/unix/mac platforms the PAL creates a cache of the system environment variables at startup since accessing environment variables is not thread safe. This means that any call the putenv or getenv by native code after process startup will be ignored in managed code or any child processes spawned by managed code.

Profilers often use environment variables to communicate with child processes or managed code, historically on Windows they could just call SetEnvironmentVariable and it would be picked up, but now on cross plat scenarios the only workaround is to use IL rewriting to inject a call to Environment.SetEnvironmentVariable, which is a substantial amount of work if the profiler is not already doing IL rewriting.

Customer impact
Without this fix profilers won't be able to coordinate with child processes while debugging. See this comment by the profiler author requesting this be included in 3.1:

@davmason Without this, we can lost all of our features related to child processes: 1) ability to profile child processes (for details see https://github.com/dotnet/coreclr/issues/23383#issuecomment-514587669), 2) ability to take snapshots automatically depending on child processes hierarchy, 3) unit-testing profiler support. Most likely, end user experience will be significantly worsened if our "tricks" stop working.

Originally posted by @k15tfu in #27157 (comment)

Fix description
Expose the PAL's implementation of SetEnvironmentVariable and GetEnvironmentVariable on the profiler interfaces.

Risk
There is very low risk (essentially no risk) to existing codepaths since the existing codepaths are unchanged. This fix only exposes new functionality and does not modify existing behavior.

@davmason davmason added this to the 3.1 milestone Oct 28, 2019
@davmason davmason self-assigned this Oct 28, 2019
@MeiChin-Tsai
Copy link

approved for 3.1. For customer unblocking.

@leecow leecow added the Servicing-approved Approved for servicing release label Oct 29, 2019
@k15tfu
Copy link

k15tfu commented Oct 30, 2019

@davmason Do you need anything from me?

@davmason
Copy link
Member Author

@k15tfu nothing is needed, it was approved and I'm waiting merge until the branch is open for checkins. The release branch is currently locked down waiting for the preview release to build

@k15tfu
Copy link

k15tfu commented Oct 30, 2019

@davmason BTW, how do you generate corprof.h without all of these extra spaces I have? I use midl src\inc\corprof.idl from VS2017 developer console.

@davmason davmason merged commit 9358413 into dotnet:release/3.1 Oct 30, 2019
@davmason
Copy link
Member Author

@davmason BTW, how do you generate corprof.h without all of these extra spaces I have? I use midl src\inc\corprof.idl from VS2017 developer console.

If you build on windows midl will run automatically and you can grab corprof.h out of the obj folder, but there's no way around the whitespace issues. I don't know why midl constantly changes whitespace but it does.

@k15tfu
Copy link

k15tfu commented Nov 11, 2019

@davmason Is there any ETA for 3.1-preview3?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants