Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CTS timeout and linked cancellation tokens #2410

Closed
pepone opened this issue Dec 30, 2022 · 1 comment · Fixed by #2411
Closed

CTS timeout and linked cancellation tokens #2410

pepone opened this issue Dec 30, 2022 · 1 comment · Fixed by #2411
Assignees
Milestone

Comments

@pepone
Copy link
Member

pepone commented Dec 30, 2022

    I don't know.

CreateLinkedTokenSource + CancelAfter creates one less disposable object and doesn't involve UnsafeRegister... so it looks cleaner.

Originally posted by @bernardnormier in #2396 (comment)

@pepone
Copy link
Member Author

pepone commented Dec 30, 2022

We should sort this out we use both patterns, and the reason to pick one or the other is not clear

The deadline interceptor and middleware use UnsafeRegister

https://github.com/zeroc-ice/icerpc-csharp/blob/1574c9ec419b5fac88a5c24eb07c170a92f0a075/src/IceRpc.Deadline/DeadlineInterceptor.cs#L79-L82

Using CreateLinkedTokenSource and CancelAfter seems simpler, the implementation of CreateLinkedTokenSource hides this UnsafeRegister call and the CanBeCanceled check

https://github.com/dotnet/runtime/blob/5b8ebeabb32f7f4118d0cc8b8db28705b62469ee/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs#L747-L748

https://github.com/dotnet/runtime/blob/5b8ebeabb32f7f4118d0cc8b8db28705b62469ee/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs#L773-L792

On the other hand, it makes sense to use UnsafeRegister to avoid allocating an additional TCS like in

https://github.com/zeroc-ice/icerpc-csharp/blob/1574c9ec419b5fac88a5c24eb07c170a92f0a075/src/IceRpc/Transports/Internal/SlicPipeWriter.cs#L82-L84

Here using UsafeRegister avoids allocating a CTS, but other than that the CreateLinkedTokenSource is similar under the hood it uses UnsafeRegister we are not paying for the synchronization context stuff

Related:

dotnet/runtime#27238

@pepone pepone added this to the 0.1 milestone Dec 30, 2022
@pepone pepone self-assigned this Dec 30, 2022
pepone added a commit that referenced this issue Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant