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

Fixed potential memory issue and message duplication with large strings #3345

Merged
merged 1 commit into from Apr 29, 2019

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Apr 29, 2019

StringBuilderPool - Allow FastPool-object to have larger capacity, but missing reset

Serious bug introduced with #2934

If writing very large log-message, then NLog will never forget it.

Will try later to make a unit-test for this special logic about releasing StringBuilders that have become too big.

@codecov-io
Copy link

Codecov Report

Merging #3345 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##             dev   #3345    +/-   ##
======================================
+ Coverage     80%     80%   +<1%     
======================================
  Files        356     356            
  Lines      28207   28205     -2     
  Branches    3750    3750            
======================================
+ Hits       22580   22607    +27     
+ Misses      4541    4515    -26     
+ Partials    1086    1083     -3

@304NotModified 304NotModified added this to the 4.6.3 milestone Apr 29, 2019
@304NotModified 304NotModified changed the title StringBuilderPool - Allow FastPool-object to have larger capacity, but missing reset Fixed potential memory issue with CSV layout Apr 29, 2019
@304NotModified 304NotModified changed the title Fixed potential memory issue with CSV layout Fixed potential memory issue with large strings Apr 29, 2019
@304NotModified 304NotModified merged commit f8f7414 into NLog:dev Apr 29, 2019
@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 30, 2019

@304NotModified The bug will be triggered by logging a very large log-message. After having logged this message, then NLog will continuously log the same very large log-message over and over.

So if logging a very large message then it will not be a "potential" issue, but a very certain issue :)

@304NotModified
Copy link
Member

304NotModified commented Apr 30, 2019

Is it a memory issue, or are the messages duplicated?

@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 30, 2019

Is it a memory issue, or are the messages duplicated?

Both. After the bug has been triggered. Then it will concat to the same buffer. Only when it finally becomes extremely large then it will reset the buffer properly.

@304NotModified
Copy link
Member

Could we please add an unit test for this? I'm curious how big the message should be as none of the tests fails.

@304NotModified 304NotModified changed the title Fixed potential memory issue with large strings Fixed potential memory issue and message duplication with large strings Apr 30, 2019
@snakefoot
Copy link
Contributor Author

Like I said, then I have been trying to make an unit-test of the StringBuilderPool ability to release too large StringBuilders.

But it requires several Threads and physical cores to properly see that it is failing. And when running in release-mode then it is almost impossible.

One could write a unit-test for the specific bug, but it would be very dependent on the magic optimization-values in the StringBuilderPool. And when changing the values then the unit-test will not be testing anything. So not that keen on doing that.

@304NotModified
Copy link
Member

Like I said, then I have been trying to make an unit-test of the StringBuilderPool ability to release too large StringBuilders.

Oops, missed that.

Busy, but will try to create a release ASAP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants