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

Auto-flush on process exit improvements #3255

Merged
merged 1 commit into from
Mar 29, 2019

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Mar 28, 2019

AsyncHelpers.StartAsyncTask defaults to ThreadPool.QueueUserWorkItem

Resolves #3200 . Tasks doesn't flush well on process-exit-event. Reverting back to NLog 4.4 behavior.

@304NotModified 304NotModified added this to the 4.6.1 milestone Mar 28, 2019
@304NotModified
Copy link
Member

304NotModified commented Mar 28, 2019

thanks!

Is this for NLog 4.6.1? (as 4.5.x and 4.6.0 are then "other behavior"?)

@snakefoot
Copy link
Contributor Author

Yes it should be safe.

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.

OK thanks!

@304NotModified 304NotModified changed the title AsyncHelpers.StartAsyncTask defaults to ThreadPool.QueueUserWorkItem Improved flush behavior on process exit Mar 28, 2019
@304NotModified
Copy link
Member

@snakefoot I try to rename the PRs to a more functional title, so it's easier to use in the changelog. Did I rename this PR correct?

@snakefoot snakefoot changed the title Improved flush behavior on process exit Auto-flush on process exit using ThreadPool instead of Tasks Mar 28, 2019
@snakefoot
Copy link
Contributor Author

@304NotModified I like to describe the technical change, when not super sure whether it is actually the cure :)

@304NotModified
Copy link
Member

I think "improved" was better then, as it's functional and it's not strict (it's fuzzy ;))

I don't think anyone cases if we use threadpools or tasks ;)

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #3255 into dev will decrease coverage by <1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##             dev   #3255    +/-   ##
======================================
- Coverage     80%     80%   -<1%     
======================================
  Files        354     355     +1     
  Lines      27972   27931    -41     
  Branches    3717    3709     -8     
======================================
- Hits       22335   22285    -50     
- Misses      4572    4573     +1     
- Partials    1065    1073     +8

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #3255 into dev will decrease coverage by <1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##             dev   #3255    +/-   ##
======================================
- Coverage     80%     80%   -<1%     
======================================
  Files        354     355     +1     
  Lines      27972   27976     +4     
  Branches    3717    3717            
======================================
- Hits       22335   22316    -19     
- Misses      4572    4595    +23     
  Partials    1065    1065

@304NotModified
Copy link
Member

I think "improved" was better then, as it's functional and it's not strict (it's fuzzy ;))

I don't think anyone cases if we use threadpools or tasks ;)

Is it OK if I would rename it back? Don't wanna have a PR title war ;)

@snakefoot
Copy link
Contributor Author

snakefoot commented Mar 29, 2019 via email

@304NotModified 304NotModified changed the title Auto-flush on process exit using ThreadPool instead of Tasks Auto-flush on process exit improvements Mar 29, 2019
@304NotModified 304NotModified added the enhancement Improvement on existing feature label Mar 29, 2019
@304NotModified 304NotModified merged commit 472a06a into NLog:dev Mar 29, 2019
@304NotModified
Copy link
Member

You just rename it as you want :)

It was tempting to rename it to "Julian is the best!1" 😄

@snakefoot
Copy link
Contributor Author

Would be more descriptive and correct, than random improvement to ProcessExit-logic :)

@snakefoot snakefoot deleted the StartAsyncTaskUserWorkItem branch April 4, 2020 12:58
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