DuplicateTypeException in CommandService

[alex91352]

When calling CommandService.Handle for the first time in parallel, it's possible for two threads to pass the !_initialized check on this line.

https://github.com/Eventuous/eventuous/blob/b72c9c0d9115c4a25572e6277c54a940e4b5b9b0/src/Core/src/Eventuous.Application/AggregateService/CommandService.cs#L57

This means that BuildHandlers can be called twice, which causes a DuplicateTypeException when attempting to add the handler to the HandlersMap.

A possible solution would be to duplicate the !_initialized check inside BuildHandlers.

[mytresor]

@alexeyzimarev I would like to submit a PR with a fix for this issue. But I cannot publish my brach (You don't have permissions to push to "Eventuous/eventuous"). Please, suggest me with my contribution. What do I need to do to get the permissions?

[alexeyzimarev]

Hey @mytresor The normal way to contribute on GitHub is using a fork https://docs.github.com/en/get-started/exploring-projects-on-github/contributing-to-a-project

[alexeyzimarev]

I am not sure moving the check down would solve as it will probably fail exactly the same way. There are two ways around it - make initialisations explicit and call it once at startup somehow (I didn't check how) or to accept the performance hit of using Interlocked.

[mytresor]

Thanks, Alexei. Sorry for a dumb question. Actually, adding an additional check inside works pretty well. I have a test project and can easily reproduce the issue with the current codebase. Adding the additional check inside of the BuildHandlers method does fix the issue (tested with 10 parallel threads). I think there is no need to use Interlocked, because the method itself is marked as [MethodImpl(MethodImplOptions.Synchronized)].

[alexeyzimarev]

Ok, I am of course willing to accept this, and you guys can verify that it works.

[alexeyzimarev]

Ah, if the function is marked as synchronised, then it will definitely work. But, it still will create an issue with performance because synchronised invocations will build up a queue. That function won't be called in parallel, only sequentially. Off it will be relatively fast, but there will certainly be some performance impact.

[alexeyzimarev]

Ok, what if instead of checking the boolean inside the synchronised function, the function itself will be idempotent? It's easy to check if handlers are already registered (I might have overlooked something), and ignore them. Then, still, the function could be called multiple times, but it won't crash. The boolean will eventually be set to true and the check will pass.

This way, we can ensure that the builder is only registering things once, and we don't get repeated calls to synchronised context, so no performance overhead.

[mytresor]

This will work for sure, and the check can be added inside of the loop. But do you really think it has such a noticeable perf impact? The BuildHandlers method will not called at all after the initialisation, so there will be no perf impact in runtime. The only time we might have some minor delays is during the initialisation (basically first few Handle calls at max).

[alexeyzimarev]

Ok, I am sorry for not reading your proposal properly. You said "add additional check" and I thought of "moving the check". Yeah, it will work.

[mytresor]

Yeah, thats right. Just a double check. https://github.com/Eventuous/eventuous/pull/331

[alexeyzimarev]

Merged, done! :)