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

LogManager.Shutdown - Should disable file watcher and avoid auto reload #2616

Merged

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Mar 8, 2018

One might consider to call LogFactory.Close(), but that might be a breaking change as it would prevent the engine to startup again after shutdown (Would have to fix unit tests, so they don't call LogManager.Shutdown)

Adventure that continues from #1899 and #82

LogManager.Shutdown() was created to simulate Shutdown in Log4net.

@snakefoot snakefoot force-pushed the LogManagerShutdownWithoutReload branch 2 times, most recently from 55af206 to 0fb2b47 Compare March 8, 2018 01:30
@codecov
Copy link

codecov bot commented Mar 8, 2018

Codecov Report

Merging #2616 into master will decrease coverage by <1%.
The diff coverage is 80%.

@@           Coverage Diff           @@
##           master   #2616    +/-   ##
=======================================
- Coverage      81%     81%   -<1%     
=======================================
  Files         326     326            
  Lines       24028   24034     +6     
  Branches     3037    3037            
=======================================
  Hits        19462   19462            
- Misses       3746    3754     +8     
+ Partials      820     818     -2

@snakefoot snakefoot force-pushed the LogManagerShutdownWithoutReload branch 3 times, most recently from 44d265b to 633230c Compare March 8, 2018 19:07
@snakefoot
Copy link
Contributor Author

snakefoot commented Mar 8, 2018

@304NotModified Improved NLogManager.Shutdown() so it performs a more correct shutdown (and also disconnects active loggers from using the closed targets)

Have also disabled the concurrent shutdown-auto-flush on the Linux-platform, as it seems to cause problems for NetCore-Applications (Just like for Mono)

@snakefoot snakefoot force-pushed the LogManagerShutdownWithoutReload branch 7 times, most recently from 9097a4e to 6c96d6f Compare March 8, 2018 22:49
@snakefoot
Copy link
Contributor Author

@304NotModified Little annoying that creating a new Logger-object actually causes the LoggingConfiguration to be reloaded after having called LogManager.Shutdown().

Have now changed the logic of LogManager.Shutdown(), so it requires an explicit assignment of LogManager.Configuration to startup again.

@snakefoot snakefoot force-pushed the LogManagerShutdownWithoutReload branch from 6c96d6f to 092b8d5 Compare March 8, 2018 23:16
@snakefoot snakefoot force-pushed the LogManagerShutdownWithoutReload branch from 092b8d5 to d6754c9 Compare March 10, 2018 12:14
@snakefoot
Copy link
Contributor Author

snakefoot commented Mar 11, 2018

Victims of shutdown-auto-flush not working on Linux (Needs explicit call to NLog.LogManager.Shutdown())

https://github.com/dotnet/corefx/issues/27952
NLog/NLog.Extensions.Logging#201

Better to have unflushed logevents than segmentation fault at application exit (even if no logevents to flush)

@snakefoot
Copy link
Contributor Author

snakefoot commented Mar 15, 2018

@304NotModified Would like to have this included in NLog 4.5 RTM if possible.

There are two important changes:

  • LogManager.Shutdown() works correctly when autoload=true
  • Avoid unhandled exceptions (segmentation fault) when running on Linux, when forgetting to call LogManager.Shutdown.

@304NotModified 304NotModified added this to the 4.5 milestone Mar 15, 2018
@304NotModified 304NotModified added the enhancement Improvement on existing feature label Mar 15, 2018
@304NotModified 304NotModified merged commit 22a439b into NLog:master Mar 15, 2018
@304NotModified
Copy link
Member

Was doubting about that, but it's merged now :)

@snakefoot
Copy link
Contributor Author

Thank you for the merge. Any update on the timeline for NLog 4.5 RTM ?

@304NotModified
Copy link
Member

Yes, in the weekend 24+25 I have some more free time and i planned the rtm release for then.

@snakefoot
Copy link
Contributor Author

Very exciting. I will be going on easter holiday from monday the 26th.

@snakefoot snakefoot deleted the LogManagerShutdownWithoutReload branch April 4, 2020 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement on existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants