jkarder commented 3 years ago

This PR updates ReceiveFrameBytesAsync to detach competing listeners before finishing the TaskCompletionSource and call TrySetResult and TrySetCanceled instead of SetResult and SetCanceled.

Fixes #965.

codecov[bot] commented 3 years ago

Codecov Report

Merging #966 (0e0c470) into master (10a263b) will decrease coverage by 0.18%. The diff coverage is 40.74%.

@@            Coverage Diff             @@
##           master     #966      +/-   ##
==========================================
- Coverage   66.10%   65.92%   -0.19%     
==========================================
  Files         148      148              
  Lines        9037     9059      +22     
  Branches     1486     1495       +9     
==========================================
- Hits         5974     5972       -2     
- Misses       2461     2472      +11     
- Partials      602      615      +13

Impacted Files	Coverage Δ
src/NetMQ/AsyncReceiveExtensions.cs	`39.60% <40.74%> (+0.36%)`	:arrow_up:
src/NetMQ/NetMQConfig.cs	`45.09% <0.00%> (-9.81%)`	:arrow_down:
src/NetMQ/Core/SessionBase.cs	`72.38% <0.00%> (-7.47%)`	:arrow_down:
src/NetMQ/Core/Patterns/Utils/FairQueueing.cs	`93.33% <0.00%> (-2.23%)`	:arrow_down:
src/NetMQ/Core/Patterns/Utils/LoadBalancer.cs	`68.42% <0.00%> (-1.76%)`	:arrow_down:
src/NetMQ/Core/Transports/StreamEngine.cs	`63.51% <0.00%> (ø)`
src/NetMQ/Core/SocketBase.cs	`71.19% <0.00%> (+0.35%)`	:arrow_up:
src/NetMQ/Core/Patterns/Dish.cs	`53.84% <0.00%> (+3.41%)`	:arrow_up:

drewnoakes commented 3 years ago

The same treatment is needed in ReceiveFrameStringAsync. I don't know why SkipFrameAsync doesn't support cancellation, but you could add it there too as the pattern is similar.

Also, it's worth checking token.CanBeCanceled, as default(CancellationToken) will never be cancelled and so there's no point registering. For many users, no cancellation will be required, so we can avoid the callback registration overhead completely.

somdoron commented 3 years ago

LGTM

abeham commented 3 years ago

The tests succeed locally on my Windows 10 installation for netcoreapp2.1, netcoreapp3.1, and net47. Maybe the build server had some hickups, because the failing test is a timeout.

jkarder commented 3 years ago

Thanks for your approvals. The tests succeed here as well (Windows 10.0.19041).

netcoreapp2.1:

Test Run Successful.
Total tests: 256
     Passed: 240
    Skipped: 16
 Total time: 37.3524 Seconds

netcoreapp3.1:

Test Run Successful.
Total tests: 258
     Passed: 242
    Skipped: 16
 Total time: 38.2665 Seconds

net47:

Test Run Successful.
Total tests: 259
     Passed: 243
    Skipped: 16
 Total time: 39.0455 Seconds

BTW, would you like me to revert the whitespace changes I accidentally committed in d297a27?

drewnoakes commented 3 years ago

Re-running jobs to see if that helps.

BTW, would you like me to revert the whitespace changes I accidentally committed in d297a27?

Sure, but only if it's convenient.

drewnoakes commented 3 years ago

Yep, just needed a kick. I'll merge as-is. Thanks very much!

zeromq / netmq

Fix cancellation in ReceiveFrameBytesAsync #966

Codecov Report