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

InternalLogger - Better support for multiple threads when using file #1927

Merged
merged 1 commit into from
Feb 5, 2017
Merged

InternalLogger - Better support for multiple threads when using file #1927

merged 1 commit into from
Feb 5, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Jan 22, 2017 •
edited
Loading

Extra lock-operation has been added when doing exclusive file-lock, to improve support for
multi-threaded use of the InternalLogger.

This change is Reviewable

@codecov-io
Copy link

codecov-io commented Jan 22, 2017 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@fc487be). Click here to learn what that means.

@@           Coverage Diff            @@
##             master   #1927   +/-   ##
========================================
  Coverage          ?     82%           
========================================
  Files             ?     285           
  Lines             ?   19545           
  Branches          ?    2285           
========================================
  Hits              ?   15951           
  Misses            ?    3048           
  Partials          ?     546

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc487be...e7271c6. Read the comment docs.

@snakefoot snakefoot mentioned this pull request Jan 22, 2017
Closed
@snakefoot snakefoot changed the title InternalLogger - Attempt to use atomic FileStream InternalLogger - Attempt to use FileStream atomic append Jan 22, 2017
@304NotModified 304NotModified added the enhancement Improvement on existing feature label Jan 22, 2017
@snakefoot snakefoot force-pushed the InternalLoggerMultiProcess branch 4 times, most recently from bb6a983 to 9dd455a Compare January 24, 2017 21:14
304NotModified
304NotModified requested changes Jan 26, 2017
View reviewed changes
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

some questions,

also not sure about this one. The internallogger should be as simple as possible? If the internallogger won't work, it's an issue.

src/NLog/Common/InternalLogger.cs Outdated
{
lock (LockObject)
{
_writeToFileAtomicSuccesCount = -1;
Copy link
Member

Choose a reason for hiding this comment

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

should be use Interlocked.Decrement?

src/NLog/Common/InternalLogger.cs Outdated
{
if (_writeToFileAtomicSuccesCount == 0)
{
if (PlatformDetector.IsMono || !PlatformDetector.IsDesktopWin32)
Copy link
Member

Choose a reason for hiding this comment

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

why we need this case?

@snakefoot
Copy link
Contributor Author

snakefoot commented Jan 27, 2017 •
edited
Loading

also not sure about this one. The internallogger should be as simple as possible?

@304NotModified I have removed the support for atomic writes, and instead just implemented a lock around the file-write. Then it will at least support multi-threaded file-logging.

@snakefoot snakefoot changed the title InternalLogger - Attempt to use FileStream atomic append InternalLogger - Improves logging behavior when used by multiple threads Jan 27, 2017
@snakefoot snakefoot changed the title InternalLogger - Improves logging behavior when used by multiple threads InternalLogger - Better support for multiple threads when using file Jan 28, 2017
@304NotModified 304NotModified modified the milestone: 4.4.2 Feb 3, 2017
@304NotModified
Copy link
Member

@304NotModified I have removed the support for atomic writes, and instead just implemented a lock around the file-write. Then it will at least support multi-threaded file-logging.

Good one!

304NotModified
304NotModified requested changes Feb 3, 2017
View reviewed changes
src/NLog/Common/InternalLogger.cs Outdated
{
lock (LockObject)
// log to LogWriter
var writer = LogWriter;
Copy link
Member

Choose a reason for hiding this comment

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

why is this code in the finally? Is this really by-design?

I only use finally to clean up stuff, never to do functional logic. Am I missing here something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanted to ensure that it also tried the logger-targets in case the file-logger failed. But I have now reverted it. Not that important to me.

@snakefoot snakefoot mentioned this pull request Feb 4, 2017
Closed
@304NotModified
Copy link
Member

:shipit:

@304NotModified 304NotModified merged commit bd7ff78 into NLog:master Feb 5, 2017
@304NotModified
Copy link
Member

Thanks for the changes. More more KISS now :)

@snakefoot snakefoot deleted the InternalLoggerMultiProcess branch October 10, 2017 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@304NotModified 304NotModified 304NotModified approved these changes

Assignees
No one assigned
Labels
enhancement Improvement on existing feature
Projects
None yet
Milestone
4.4.2
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@snakefoot @codecov-io @304NotModified