Cancellation

[mycroes]

@gfoidl Would you care to review this?

[mycroes]

@scamille Thanks for the initial review!

[scamille]

My pleasure. I am quite out of the loop with .NET and also never used the new memory facilities much, so I can't really cover everything. I mainly skimmed over things, and used this to ask some interesting questions :-)

But in general the PR looks great. From a usability perspective, having both user cancellation and proper working timeouts is definitely a very very useful thing to have in this async network code, in particular if you want to scale things up (from a number of connections/devices/queries perspective).

[mycroes]

My pleasure. I am quite out of the loop with .NET and also never used the new memory facilities much, so I can't really cover everything. I mainly skimmed over things, and used this to ask some interesting questions :-)

Well this was my first attempt at using memory as well, I think Span actually was one of the reasons for me to start the library. However I just learned I'm not that knowledgeable either. As arraypool-vs-memorypool-minimizing-allocations-ais-dotnet points out there's a difference between ArrayPool and MemoryPool, the latter of which actually has the dispose pattern you were wondering about. In short, I'd probably be better off using ArrayPool instead of MemoryPool, so I'll get on that as well.

But in general the PR looks great. From a usability perspective, having both user cancellation and proper working timeouts is definitely a very very useful thing to have in this async network code, in particular if you want to scale things up (from a number of connections/devices/queries perspective).

The reason for this PR is that it was utterly broken. The cancellation was fine up to a certain point, but not every target framework could make proper use of it. As a result, I had a read action that never finished. Adding the request timeout will handle part of that problem, but it still doesn't cancel a socket read on net461 for instance. Adding the Close calls in the right places solves this though, as does running this on net5 or newer. We do have a lot of customer projects running on .NET Framework though, having an updated Sally7 that fixes this problem is the easiest way to handle this (versus updating the target frameworks, which won't always be a realistic option).

[gfoidl]

difference between ArrayPool and MemoryPool, the latter of which actually has the dispose pattern you were wondering about.

For the IDisposable-allocation with the MemoryPool we mitigated this with the Sally7MemoryPool (introduced in https://github.com/mycroes/Sally7/pull/23).

[mycroes]

For the IDisposable-allocation with the MemoryPool we mitigated this with the Sally7MemoryPool (introduced in #23).

You're right. It does mean we can't track use-after-free though (which you can do with allocation per lease), which can happen if the S7 server misbehaves. I'm not really worried about that, but perhaps it would still be a good idea to fix that at some point. However I'd rather fix that by adding some additional checks on the Request type instead.

[mycroes]

Thanks @gfoidl, some great comments, will make some changes.

[mycroes]

Made all the recommended changes, if you guys would be willing to do another review that would be awesome. I intend to release this shortly, the what-if fixes for the ConcurrentRequestExecutor can come at a later time.

[mycroes]

Thanks @gfoidl, again, I appreciate it a lot that you took the time to review this. Also @scamille thanks again for initial review! I hope you guys will stick around and provide feedback in the future as well.

[gfoidl]

stick around and provide feedback in the future as well.

Sure! Nice work is happening here 😃.