[QUIC] Fix native crashes and heap corruption #74611
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThere were races between disposing SendBuffers and MsQuic handles and using them from parallel threads. Most prominent in stress tests in configuration After the change, I didn't see any crashes in stress in a couple of hours, will keep them running in the meantime. Change has no impact on perf. Fixes #72696
|
| } | ||
| } | ||
|
|
||
| public void SafeCall(Action<MsQuicSafeHandle> call) |
There was a problem hiding this comment.
This generally should not be need. runtime would automatically grab reference on pinvoke if we pass in safe handle. .... But we don't. It seems like we lost that when switching to generated API. This is all hidden as we pass the safe handle around everywhere and then we don't use it when we should and we pass IntPtr to quic.
There was a problem hiding this comment.
I am not thrilled about the fact that this leads to closure allocation per call in many cases (StreamSend, EnableReceive, ....). I think we should consider (possibly in follow-up) either
- writing wrappers for MsQuicApi which does this for us (i.e. duplicating what P/Invoke does)
- look for a way to have the source generator generate them for us.
There was a problem hiding this comment.
I agree with both comments, and I believe we should look for a way to solve this in generated API, otherwise it is not sustainable. If we would be able to do that in time, it would be ideal.
| @@ -90,6 +90,7 @@ public sealed partial class QuicStream | |||
| } | |||
| }; | |||
| private MsQuicBuffers _sendBuffers = new MsQuicBuffers(); | |||
| private object _sendBuffersLock = new object(); | |||
There was a problem hiding this comment.
Discussed offline; it's either this or make MsQuicBuffers class instead of struct
|
There's a branch with a different approach to safe calls via "generated-like" interop; discussed with @ManickaP that it will be her choice with which solution to proceed UPD: opened alternative PR with that branch: #74669 |
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThere were races between disposing SendBuffers and MsQuic handles and using them from parallel threads. Most prominent in stress tests in configuration After the change, I didn't see any crashes in stress in a couple of hours, will keep them running in the meantime. Change has no impact on perf. Fixes #72696
|
|
Closing in favor of #74669 |
…terop (dotnet#74669) * Send buffers and handles crash fixes * Add generated-like interop * Apply PR feedback from dotnet#74611 * Change asserts * Feedback + moved native methods to their own file * PR feedback Co-authored-by: ManickaP <mapichov@microsoft.com>
…terop (#74669) (#75192) * Send buffers and handles crash fixes * Add generated-like interop * Apply PR feedback from #74611 * Change asserts * Feedback + moved native methods to their own file * PR feedback Co-authored-by: ManickaP <mapichov@microsoft.com> Co-authored-by: ManickaP <mapichov@microsoft.com>
ghost
locked as resolved and limited conversation to collaborators
There were races between disposing SendBuffers and MsQuic handles and using them from parallel threads. Most prominent in stress tests in configuration
-cancelRate 1 -ops 9(POST Duplex Dispose).After the change, I didn't see any crashes in stress in a couple of hours, will keep them running in the meantime.
Change has no impact on perf.
Fixes #72696