GeezerVison

By having FileShare.ReadWrite, we allow the file to be opened for writing by another process. FileShare.ReadWrite could only possibly work in a logging scenario, where new content is always appended to the end of a file and you read the file through seek. I think it should be changed to FileShare.Read.

With FileShare.Read the PowerShell Core process will terminate if anybody open the file for writing. We aim to avoid this and open the file with minimal lock level. Also we don't keep the file opened: every time we create new StreamReader on very short time. There still can be a race condition if we read partially changed file but it is another issue - what PowerShell Core should do if the config file is broken.

IMO, this permission raises more confusion than the potential race condition it might mitigate. I think the right approach would be to return a default setting when the file cannot be opened after the retry, or when the file is corrupted (schema validation would be good but that would impact the startup time).

The current design is to only read the particular setting that is requested, which will result in opening the files multiple times but actually reading the whole file every time. I don't think this is the right thing to do. We probably should be very explicit that the setting file is read only one time at startup, and we process all settings that is present in the file. If the file cannot be opened after retry or the content is corrupted, we use the default settings and somehow convey the error to user (yes, we need to define the default settings first). In this way, the file is read only once, which means less race conditions, and less file access cost. Anyways, this should be a separate PR, not a change request for this PR. Here we only focus on the share permission.

Should I change anything in the PR?

IMO, this permission raises more confusion than the potential race condition it might mitigate. I think the right approach would be to return a default setting when the file cannot be opened after the retry, or when the file is corrupted (schema validation would be good but that would impact the startup time).

Yes, I think changing FileShare.ReadWrite to FileShare.Read :)

I am not sure that this resolve the race condition in our xUnit tests - I started the PR to get this.

Your xUnit tests actively modify the config file (write lock?) and this lead to crash other tests .
I agree that here we can only temporary resolve the problem and we should work on better solution. (Address in my RFC or make PR?)

Dongbo Wang (@daxian-dbw) returning a default settings file could result in security settings in the config file being bypassed.

Your xUnit tests actively modify the config file (write lock?) and this lead to crash other tests .

Ilya (@iSazonov) that's unfortunate to hear :( Then let's go with FileShare.ReadWrite for now.

Travis Plunk (@TravisEz13) I agree. It sounds to me crashing pwsh when config file parsing fails is more securer :)

Dongbo Wang (@daxian-dbw) Travis Plunk (@TravisEz13) Could you please add more feedback about "security settings" to my RFC? We could discuss this there. I don't like the "crashing pwsh" because it could be a DoS attack even by admin user.