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

NLogConfig - TouchAndApplyNLogConfig with Exception handler #2846

Merged
merged 1 commit into from
Apr 9, 2020

Conversation

snakefoot
Copy link
Contributor

Please follow the guide below

  • Searched for similar pull requests
  • Compiled the code with Visual Studio
  • Require translation update
  • Require document update (readme.md, wikipage, etc)

What is the purpose of your pull request?

  • Bug fix

Description

Avoid application crash when not having file permissions to setup default NLog.config. See also #2841

@ghost ghost requested a review from celeron533 April 5, 2020 11:42
@celeron533
Copy link
Contributor

Thanks @snakefoot , is has been fixed in 5bc4a0b

return;
}

LoadConfiguration(); // Load the new config-file
Copy link
Contributor

Choose a reason for hiding this comment

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

There may not have the NLog config file due to the previous write permission issue.

Copy link
Contributor Author

@snakefoot snakefoot Apr 6, 2020

Choose a reason for hiding this comment

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

It only performs the LoadConfiguration if it was successful in creating a new one. So if it can write to the location, then it should also be able to read.

Copy link

Choose a reason for hiding this comment

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

So the routine is like this? I think this change is ok then.

When write error: Line 113->116 throw error ->118 catch error->120->121 return
When config exist: (config auto loaded by NLog itself at program start) 113->114 return
When config not exist: 113->116->124 load config->return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it returns without doing anything for all cases. Except for the very first time where it successfully have created a new default NLog.config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the 5bc4a0b is committed to fix the CurrentDirectory issue. The NLog config file is always create/read/write on the same place in file system. Will merge the code soon.

PS:
Before the previous autostart issue fix, SS may create the NLog config file in the wrong place, such as System folder and request the NLog to load it.
Then the following code change the CurrentDirectory to Application.StartPath

Directory.SetCurrentDirectory(Application.StartupPath);

Which made the NLog file location inconsistent. When user change the log level and NLog need to reload the config file, may fail.

@celeron533 celeron533 merged commit 6767916 into shadowsocks:master Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants