Add cancellation token support to Subscribers#912
Add cancellation token support to Subscribers#912yang-xiaodong merged 4 commits intodotnetcore:masterfrom
Conversation
matt-psaltis
force-pushed
the
feature/cancellationTokens
branch
from
3573ddc to
81120de
Compare
matt-psaltis
force-pushed
the
feature/cancellationTokens
branch
from
81120de to
e7ad0d4
Compare
yang-xiaodong
left a comment
yang-xiaodong
left a comment
There was a problem hiding this comment.
Hello @matt-psaltis,
Thank you very much for your PR, I reviewed and found two issues, please confirm it
|
|
||
| public AsyncLocal<ICapTransaction> Transaction { get; } | ||
|
|
||
| public void Start(CancellationToken stoppingToken) |
There was a problem hiding this comment.
Should it be removed? I think the prevent Start method in CapPublisher looks a bit strange? and the method is not called
There was a problem hiding this comment.
It does look strange on the publisher side agreed. I need to come back to this and reassess. It didn't quite fit the current structure.
|
|
||
| var methodInfo = context.ConsumerDescriptor.MethodInfo; | ||
| var reflectedType = methodInfo.ReflectedType.Name; | ||
| var reflectedType = methodInfo.ReflectedType.TypeHandle.Value; |
There was a problem hiding this comment.
If we use ReflectedType.TypeHandle.Value, as described in #908, do we still need "{methodInfo.Module.Name}_{reflectedType}_{methodInfo.MetadataToken}" to ensure that the key is unique?
There was a problem hiding this comment.
Hey got slammed with work this week. I was trying to keep the PRs separate so I was planning to do this once I had some test coverage in place in a subsequent PR. happy to amalgamate tho if that's easier.
|
Hello @matt-psaltis , Any further response on this PR? |
@yang-xiaodong I've pushed some updates and additional tests for the cache key problem from #908 Would love your thoughts when you can please. |
|
Hello @matt-psaltis , Since the object that implements https://github.com/dotnetcore/CAP/blob/master/src/DotNetCore.CAP/CAP.ServiceCollectionExtensions.cs#L47 |
Awesome! I actually did go down that path today. I removed it because I wasn't sure on the intention of a "processing server" as a concept vs the dispatcher. I'll consolidate, it was cleaner that way, thanks. |
I remember why I unwound this now, it was because of the number of places that directly reference the IDispatcher. I could register it as an IProcessingServer using something like this: Need to make sure we only get a single instance created so by registering it as an IProcessing Server as well as IDispatcher, we need to use a factory pattern like above to reference the same IDispatcher instance. Does that make sense? |
|
It’s a good idea to use a factory to resolve, just do it |
Done. |
|
Looks good, thanks very much |
2 participants
Relates to: #907
Wanted to get your thoughts on this currently. I've added an example integration test as well as some additional unit tests for the subscriber resolution when cancellation tokens are in play. There's a couple of signature changes that would be potentially breaking so I'd like to get your thoughts on those please?