WebServiceTarget - FlushAsync - Avoid premature flush

[snakefoot]

Keeps track of pendingRequests. Tries to fix #259. Instead of expecting async-target to almost handle it #1853

First idea was to keep track of each individual LogEvent (in a dictionary), but the logic (and the allocations) became a little heavy. Now it just keeps track of a pending request count, so it is not completely correct when races occurs, but I think it is good enough.

---

This change is 

[codecov-io]

Current coverage is 81% (diff: 92%)

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

@@             master      #1868   diff @@
==========================================
  Files           276        277     +1   
  Lines         18634      18748   +114   
  Methods        2861       2873    +12   
  Messages          0          0          
  Branches       2139       2148     +9   
==========================================
+ Hits          15138      15260   +122   
- Misses         2977       2984     +7   
+ Partials        519        504    -15   

Powered by Codecov. Last update 2216fff...89a8519

[304NotModified]

don't we need a lock here?

[snakefoot]

FlushAsync is already protected by this.SyncRoot. Same lock-object is used for the other manipulations. But I can add an explicit lock, so it will not confuse.

[304NotModified]

Missed that, thx! No need to add it IMO then

[304NotModified]

great that we could close that old issue :)

Some questions, see code notes

[304NotModified]

should users also do a triple-flush?

[snakefoot]

No triple flush is only for code-coverage :)

[304NotModified]

sounds we should have split those tests.

[snakefoot]

Done

[304NotModified]

isn't HttpWebRequest an expensive key value?

[304NotModified]

Another way would be using a linkedlist and send the info with the AsyncContinuation (if possible)?

[snakefoot]

isn't HttpWebRequest an expensive key value?

I was hoping it was acting like a normal class, and just used default class-reference-hashcode.

Another way would be using a linkedlist

Yet another option is just to monitor the last written LogEvent.

And yet yet another option is to have a single counter of outstanding requests, and then keep a queue of flush-callbacks. When a flush is requested one caches the pending request count, and when it reaches zero, then it fires the flush-callback. Then it would only be a list of pending flush-callbacks and a pending-counter.

[304NotModified]

I was hoping it was acting like a normal class, and just used default class-reference-hashcode.

yes 're right!

// The default implementation returns the sync block index for this instance.
// Calling it on the same object multiple times will return the same value, so
// it will technically meet the needs of a hash function, but it's less than ideal.
// Objects (& especially value classes) should override this method.

https://referencesource.microsoft.com/#mscorlib/system/object.cs,4de9cf234d0d8b16

[snakefoot]

Have now also added AutoFlushTargetWrapper into the mix, since #259 also complains that using the AutoFlushTargetWrapper didn't solve their problem.

Now I also have the crutch (AutoFlushTargetWrapper) that people can use when having a target without a proper flush-method.

Have decided not to keep track of the individual logevents, but just have a pending-counter, and then just keep track of the pending flush-requests instead.

[304NotModified]

Reviewed 15 of 17 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

tests/NLog.UnitTests/Targets/WebServiceTargetTests.cs, line 626 at r3 (raw file):

                    CountdownEvent = new CountdownEvent(expectedMessages);
                else
                    CountdownEvent = null;

nice one :)

---

Comments from Reviewable

[snakefoot]

@304NotModified Think I have broken something, as the test-builds are now failing quite frequently, after the merge.

Have found the problem. I had broken broken the wait-counter for the two tests. See #1903