search

dolittle / Home

Dolittle is a platform designed to build Line of Business applications without sacrificing architectural quality, code quality or scalability.
http://www.dolittle.io
MIT License
27 stars 5 forks source link

CommandContextManager is misleading #82

Open smithmx opened 4 years ago

smithmx commented 4 years ago

First, this is not allowed:

https://github.com/dolittle-runtime/Runtime/blob/d1c59c5546ac53a1d4ffe5fa03e0ce4cc80a32a4/Source/Commands.Coordination/CommandContextManager.cs#L13

You cannot use ThreadStatic in the context of a web application. At best it should be AsyncLocal.

However, a bigger problem is the idea of enlisting multiple commands within a single command context. A command is a transaction, you cannot have multiple commands, effectively multiple transactions, within a single context. Every command should create its own context.

┆Issue is synchronized with this Asana task

jakhog commented 4 years ago

Just for correctness - this file has now been moved to SDK as part of the v5 changes. So the new location is here: https://github.com/dolittle-runtime/DotNET.SDK/blob/90d4657f0ada16f6462bb8f93e56ef1c97fc3af8/Source/Commands.Coordination/CommandContextManager.cs#L13

jakhog commented 4 years ago

The ThreadStatic problem has been fixed in https://github.com/dolittle-runtime/DotNET.SDK/commit/4f81d961cf81dc83d57e595109da08e832951d00.

As for multiple commands being enlisted in a single command context, I don't really understand the problem you are describing. If you establish a CommandContext for a new command - i.e. not the Command that was used to establish the current CommandContext, it will create a brand new one. So there will only ever be a single Command in a CommandContext, as you can also see here: https://github.com/dolittle-runtime/DotNET.SDK/blob/5.0.0/Source/Commands.Coordination/CommandContext.cs.

So could you please clarify what you mean with the second part of your issue?

smithmx commented 4 years ago

Ah, ok. I could have been clearer.

There can be no nesting of commands so there's no need for a command context manager that keeps track of nested commands. The only use for the CommandContextManager would be to enable this as far as I can see and we definitely don't want that.

Neither is there, for that matter, use for a command context that can track more that one aggregate.