Fix: Deadlock in NetworkTarget

[kt1996]

Deadlock in NetworkTarget Issue. fixes #1058

[codecov-io]

Current coverage is 73.03%

Merging #1110 into master will increase coverage by +0.07% as of 373faaa

@@            master   #1110   diff @@
======================================
  Files          263     263       
  Stmts        14967   14967       
  Branches      1633    1632     -1
  Methods          0       0       
======================================
+ Hit          10920   10931    +11
+ Partial        421     420     -1
+ Missed        3626    3616    -10

Review entire Coverage Diff as of 373faaa

---

Uncovered Suggestions

1. +0.09% via ...nternal/AspHelper.cs#206...218
2. +0.09% via ...nternal/AspHelper.cs#188...200
3. +0.09% via ...nternal/AspHelper.cs#170...182
4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

[304NotModified]

Changes without whitespace changes : https://github.com/NLog/NLog/pull/1110/files?w=1

Can you explain me why the declaration should be outside the lock? Then I can make sure we won't make the same mistake :)

[kt1996]

All line numbers with respect to
https://github.com/kt1996/NLog/blob/master/src/NLog/Targets/NetworkTarget.cs
https://github.com/kt1996/NLog/blob/master/src/NLog/Internal/NetworkSenders/TcpNetworkSender.cs

Debugging Findings-
Well actually on debugging I found out that the deadlock was caused by the main thread holding the lock to the TCPNetworkSender.openNetworkSender (NetworkTarget, Line 277) and waiting for lock on TCPNetworkSender instance (Multiple positions where lock(this)is present), whereas the worker thread had a lock on the TCPNetworkSender instance (TCPNetworkSender, Line 202) and was waiting for a lock on TCPNetworkSender.openNetworkSender (this was in the delegate passed on NetworkTargets, Line 318).

Reason
Note: Why this was happening was clear on going through the call stack and was the case when the toSend(Networktarget, Line 455) became zero and the sendNextChunk was invoked at a moment when the main thread was holding one lock and about to lock the other object too. Further description-
Detailed FlowThrough
The main thread held the TCPNetworkSender.openNetworkSender lock and the worker thread had entered the SocketOperationCompleted method (thus acquiring the TCPNetworkSender Lock) with the e.UserToken not null, it proceeded with asyncContinuation(this.pendingError) (a few lines later) and invoked the delegate (the sendNextChunk one, NetworkTarget, Line 460) and this time proceeded to invoke the delegate provided via (NetworkTarget, Line 470) which next waited on the lock toTCPNetworkSender.openNetworkSender. The main thread on the other hand waited for lock on TCPNetworkSender instance (TCPNetworkSender, Multiple positions where lock(this)is present, Like Line 227) and thus the deadlock situation arose.

Lock Scope Changes
I removed the declaration from the scope of lock as it was not needed, the only place the call might further on need to access openNetworkSenders was already protected by a call to lock (networkTargets, Line 318) in the delegate passed and now since the main thread was no longer unnecessarily holding the lock there was no issue of deadlock.

The only thing wrong here was unnecessarily locking of an object by the main thread which caused the deadlock

Hope I provided enough details, please ask again if u need any more info. Thanks!!

[304NotModified]

Thanks for the info!

I see now indeed that the scope of the lock has been changed.

[304NotModified]

Thanks for this bugfix. I has been merged and will be released in 4.3 :)