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.

Sign up for GitHub

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

Jump to bottom

Allow ConfigurationManager to load when GetEntryAssembly returns null. #32195

Merged
merged 6 commits into from
Feb 24, 2020
Merged

Allow ConfigurationManager to load when GetEntryAssembly returns null. #32195

merged 6 commits into from
Feb 24, 2020
+72 −21

Conversation

eerhardt
Copy link
Member

Assembly.GetEntryAssembly() will return null when the current process was loaded from a custom/native host. ConfigurationManager should support this scenario, and not throw a PNSE.

On Windows, attempt to find the native host to maintain behavior from netfx.

Fix #25027

(Note: probably easiest to review each commit separately. And with whitespace off.)

@eerhardt eerhardt force-pushed the ConfigurationChange branch from a35fd47 to 848de30 Compare February 13, 2020 00:13
@eerhardt eerhardt marked this pull request as ready for review February 13, 2020 00:14
safern
safern approved these changes Feb 13, 2020
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM

@eerhardt eerhardt force-pushed the ConfigurationChange branch from 848de30 to e65c559 Compare February 14, 2020 15:45
jkotas
jkotas reviewed Feb 15, 2020
View reviewed changes
...ries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs Outdated
{
ApplicationUri = uri.LocalPath;
applicationFilename = uri.LocalPath;
if (uri.IsFile)
Copy link
Member

Choose a reason for hiding this comment

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

When is this condition going to be false? As far as I can tell, the above should always going to produce an a filename.

(I understand that you have not added this, but since you are cleaning this up...)

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like when this code was originally written (dotnet/corefx@7d0bf61), it was using new Uri(exeAssembly.CodeBase). It was then changed by https://github.com/dotnet/corefx/pull/20488/files to use Path.Combine(AppDomain.CurrentDomain.BaseDirectory, exeAssembly.ManifestModule.Name). But the if (uri.IsFile) was left.

I can remove the check, and add an assert instead.

FYI - @JeremyKuhne

ericstj
ericstj reviewed Feb 18, 2020
View reviewed changes
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

We had discussed allowing the host to specify this via AppContext property bag, I know that wasn't strictly necessary, but would provide a replacement for AppDomainSetup.ConfigFile.

...ries/System.Configuration.ConfigurationManager/src/System/Configuration/ClientConfigPaths.cs Outdated
// Try to find the native entry point.
using (Process currentProcess = Process.GetCurrentProcess())
{
ApplicationUri = currentProcess.MainModule.FileName;
Copy link
Member

Choose a reason for hiding this comment

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

I think MainModule can be null.

JeremyKuhne
JeremyKuhne reviewed Feb 18, 2020
View reviewed changes
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Looks good to me. Presuming @ericstj null comment is ok.

@eerhardt eerhardt force-pushed the ConfigurationChange branch from caedc60 to e200bea Compare February 22, 2020 21:02
@eerhardt
Copy link
Member Author

We had discussed allowing the host to specify this via AppContext property bag, I know that wasn't strictly necessary, but would provide a replacement for AppDomainSetup.ConfigFile.

My thinking is that we can add that later if/when it is asked for. We can always add it, but if we put it in now, we wouldn't be able to remove it. I would rather wait until it is required.

@ericstj
Copy link
Member

ericstj commented Feb 24, 2020

we can add that later if/when it is asked for

I see, so long as the original issue from #25027 is addressed, along with the issue we were discussing from WPF (dotnet/wpf#2395) then I agree.

@eerhardt eerhardt merged commit 40d4b20 into dotnet:master Feb 24, 2020
@eerhardt eerhardt deleted the ConfigurationChange branch February 24, 2020 22:52
@jkotas jkotas mentioned this pull request Jul 8, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern approved these changes

@ericstj ericstj ericstj left review comments

@JeremyKuhne JeremyKuhne JeremyKuhne left review comments

@jkotas jkotas jkotas left review comments

@stephentoub stephentoub stephentoub left review comments

@maryamariyan maryamariyan maryamariyan approved these changes

Assignees
No one assigned
Labels
area-System.Configuration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ConfigurationManager is unusable when called from unmanaged code
7 participants
@eerhardt @ericstj @stephentoub @maryamariyan @jkotas @JeremyKuhne @safern