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

Fix deadlock on Factory #1223

Merged
merged 9 commits into from
Feb 7, 2016
Merged

Fix deadlock on Factory #1223

merged 9 commits into from
Feb 7, 2016

Conversation

304NotModified
Copy link
Member

  • No locking on factory when rendering layout renderers
  • Less locking when using GlobalDiagnosticsContext.Get / MappedDiagnosticsContext.Get / MappedDiagnosticsLogicalContext.Get
  • added for (indirect) locks
  • move ConvertToString from GlobalContext to FormatHelper (internal)
  • introduced StringBuilderExt, remove duplicate code (internal)

fixes #1203

@codecov-io
Copy link

Current coverage is 74.00%

Merging #1223 into master will increase coverage by +0.01% as of 8cf1773

@@            master   #1223   diff @@
======================================
  Files          265     266     +1
  Stmts        15311   15325    +14
  Branches      1682    1684     +2
  Methods          0       0       
======================================
+ Hit          11329   11341    +12
- Partial        418     420     +2
  Missed        3564    3564

Review entire Coverage Diff as of 8cf1773

Uncovered Suggestions

  1. +0.08% via ...argets/FileTarget.cs#1704...1716
  2. +0.08% via ...gingConfiguration.cs#282...293
  3. +0.08% via ...gingConfiguration.cs#244...255
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@bhaeussermann bhaeussermann mentioned this pull request Feb 7, 2016
Closed
s-sreenath
s-sreenath reviewed Feb 7, 2016
View reviewed changes
src/NLog/LayoutRenderers/MdlcLayoutRenderer.cs
{
formatProvider = LoggingConfiguration.DefaultCultureInfo;
}
builder.Append(Convert.ToString(o, formatProvider));
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it benefit if we add this logic to the base class and have it is a property?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea :)

@304NotModified
Copy link
Member Author

@page-not-found I removed the code duplication.

@304NotModified 304NotModified added this to the 4.3 milestone Feb 7, 2016
@s-sreenath
Copy link
Contributor

I did not find any other issues with this change.

@304NotModified
Copy link
Member Author

Thanks @page-not-found !

304NotModified added a commit that referenced this pull request Feb 7, 2016
@304NotModified
Merge pull request #1223 from NLog/fix-lock-context-renders
5f5942a 
Fix deadlock on Factory
@304NotModified 304NotModified merged commit 5f5942a into master Feb 7, 2016
@304NotModified 304NotModified deleted the fix-lock-context-renders branch February 7, 2016 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@s-sreenath s-sreenath

Labels
None yet
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@304NotModified @codecov-io @s-sreenath