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

RetryingWrapper - Allow closing target, even when busy retrying #1926

Merged
merged 1 commit into from
Jan 26, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Jan 22, 2017

If wrapped target is working on retrying for ever, then it should not block one from closing target.

This is a breaking change, as the RetryingWrapper is not sealed. Has to wait for NLog 5.0


This change is Reviewable

@snakefoot snakefoot force-pushed the RetryingWrapperNonBlockClose branch 3 times, most recently from d338aff to 7a148f3 Compare January 22, 2017 09:59
@codecov-io
Copy link

codecov-io commented Jan 22, 2017

Current coverage is 82% (diff: 92%)

Merging #1926 into master will increase coverage by <1%

@@             master      #1926   diff @@
==========================================
  Files           285        285          
  Lines         19496      19535    +39   
  Methods        2961       2962     +1   
  Messages          0          0          
  Branches       2277       2286     +9   
==========================================
+ Hits          15879      15927    +48   
+ Misses         3072       3060    -12   
- Partials        545        548     +3   

Sunburst

Powered by Codecov. Last update 6ba58c8...63493c4

@304NotModified 304NotModified added the enhancement Improvement on existing feature label Jan 26, 2017
@304NotModified 304NotModified merged commit 6b9c2cc into NLog:master Jan 26, 2017
@304NotModified
Copy link
Member

And thanks again!!

@304NotModified 304NotModified added this to the 4.4.2 milestone Jan 26, 2017
@304NotModified
Copy link
Member

This is a breaking change, as the RetryingWrapper is not sealed. Has to wait for NLog 5.0

Oops missed that one.

@304NotModified
Copy link
Member

304NotModified commented Jan 26, 2017

isn't .RetryingWrapper.Write calling RetryingWrapper.WriteAsyncThreadSafe (inidrect) and so it's a non-breaker?

  • WriteAsyncThreadSafe calls Write in Target

@304NotModified
Copy link
Member

304NotModified commented Jan 26, 2017

#1933 should fix this IMO. I assume that the sleep-retry logic is safe without lock.

@snakefoot
Copy link
Contributor Author

#1933 should fix this IMO. I assume that the sleep-retry logic is safe without lock.

@304NotModified When overriding WriteAsyncThreadSafe(), then it will never reach the normal Write(). This is intended as the normal Write() is called after having taken target-lock. It is the target-lock I want to avoid as it blocks one from flushing and closing the target. If you re-introduce the RetryingTargetWrapper-Write() again then it will never be reached.

The sleep-retry-logic was before protected against reordering by the Write-Target-lock, with this change then it is protected against reordering by the RetrySyncObject-lock.

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.

None yet

3 participants