Enable the transactional session to be used within the message handling pipeline

[danielmarbach]

• Addresses Ability to use ITransactionalSession via dependancy injection in an incoming message context #65

This would allow using the transactional session as an API within the handling pipeline as well as outside the handling pipeline. With that in place, the uniform session could be deprecated because the transactional session has a more powerful API by also exposing the synchronized storage consistently.

The caveats are that it is still only possible to use the session after the load handlers connector has executed inside the invoke handler stage.

Additionally, this would not execute the customizations for that session because customizations are really only ever useful for sessions that are actually opened while this one is implicitly opened. Customizations are also only useful to map some information to the concrete session options (such as partition keys etc.) while in the incoming pipeline this information is already available by using the standard mechanisms of the corresponding persister. For example, CosmosDB partition keys have to be extracted from the incoming message by using the cosmos DB transaction mapping API which then makes that information available. There is no need to tell the transactional session API where to retrieve that information from.

[SzymonPobiega]

LGTM. The only issue I can see is that we have always tried to dissuade people from using the UniformSession stating that it is not the best way of structuring your code. Because this new feature is in the same package as transactional session, it might be harder to convey the same message i.e. that the new code should not really inject the session thing everywhere.

[danielmarbach]

@SzymonPobiega I do agree that is slightly problematic but I think the history shows that the customers that want to do it will find ways to get it done via weird things like async local etc. which has all sorts of other implications. Given that currently this is a dedicated package that requires opt-in I think it should be fine.

Another question. I'm planning to organize the files a bit better in the root folder. I was thinking about moving all the pipeline related session stuff into a dedicated folder but struggle to come up with a good name / distinction. Should I call the folder "Pipeline" or "InPipeline" "WithinPipeline" or something like that?

[tmasternak]

@danielmarbach what is the transactionality guarantee between messaging operations done in a handler via context and transactionalSession? Do I understand it correctly that Sends and Publishes done via context are not "attached" to the Outbox record if one is configured?

[danielmarbach]

@tmasternak they are automatically attached. The outbox transaction is added to the context bag extension part. This gets then used when present to enlist the synchronized storage into the outbox transaction. So they are "attached" to the outbox.

[timbussmann]

code looks fine to me.

tbh, I'm not super excited about this functionality which requires quite a bit of DI trickery to basically offer the same features that the core APIs or the uniform session already do but I understand that there are scenarios where it's helpful to work with a single interface due to code reuse so I can't come up with a strong reason against it.

[timbussmann]

really wondering whether we should introduce a core feature than can do that.

[timbussmann]

so when using the pipeline aware session, users shouldn't call commit because that would then throw when the pipeline behavior tries to commit the session. I'm kinda wondering whether this is clear enough to the user (since this code isn't visible to them at all).

(sidetracking here, but I think this is a downside of the current API where both the lifetime management of the session, as well as subsequent session usages both start with injecting the session and therefore don't provide any visibility into the state of the session [has it been opened? who is managing the commit?])

[danielmarbach]

Ideally, yes we would have split the interface. That being said, this is nothing new when you look for example at DBConnections. They also have lifetime management methods such as open, close, dispose and some other methods that allow to interact with the connection in other ways.

[danielmarbach]

so when using the pipeline aware session, users shouldn't call commit because that would then throw when the pipeline behavior tries to commit the session. I'm kinda wondering whether this is clear enough to the user (since this code isn't visible to them at all).

The thing is in the near future, the majority of the lifetime management will be moved into some sort of filter pipeline things even for the controllers. So in essence for all those scenarios the user will no longer manage the session lifetime anyway. This just does that already by implementing the automatic management of the session in the pipeline.

Happy to change the implementation though if you prefer that to allow multiple commits. I do prefer though the current way because it would make discover invalid usages early on during development

[danielmarbach]

tbh, I'm not super excited about this functionality which requires quite a bit of DI trickery to basically offer the same features that the core APIs or the uniform session already do but I understand that there are scenarios where it's helpful to work with a single interface due to code reuse so I can't come up with a strong reason against it.

The thing is there is no way to uniformly get access to synchronized storage because the uniform session doesn't provide that deliberately. This code enables to share the connection means both for synchronized storage and outbox interaction in a common way which today is not possible. This is a clear win over uniform session and core combination

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[danielmarbach]

@tmasternak @timbussmann any further thoughts? I was hesitant to just merge it because there where still discussions. I think this would be a nice addition

[timbussmann]

I feel this is quite a significant feature but it also comes with some rough edges with a hard-to-predict impact (most specifically the session "customization" and the commit usage that I commented on). While I don't see a major problem with those, I'm not sure I'm able to decide with confidence that this won't be an issue. It would be helpful if we could get this as a prototype to users that would like to have this feature for them to confirm that this works as expected / does what's needed, before doing an official release?

[ramonsmits]

LGTM. The only issue I can see is that we have always tried to dissuade people from using the UniformSession stating that it is not the best way of structuring your code. Because this new feature is in the same package as transactional session, it might be harder to convey the same message i.e. that the new code should not really inject the session thing everywhere.

I disagree, UniformSession is not using DI where TransactionalSession is. IMHO it is great for pretty much all repo/service like designs users might have. 100% better compared to requiring to pass the context into the call stack passing it through layers that don't do anything with that dependency.

[ramonsmits]

@danielmarbach How about customers that would want to have an NHibernate ISession in the scope or a DbContext? I guess that would be possible to resolve as long as the resolving happens after the load handlers connector if I'm not mistaken? Pretty sure no user would want to have their objects want to take a dependency on ITransactionalSession unless that code is dispatching messages.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[danielmarbach]

Bump

[SzymonPobiega]

What's the plan on that @danielmarbach ? Could we work on it as part of the rotation group 🤔

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[danielmarbach]

@SzymonPobiega good question. I'm happy to merge it as is and do a WIP PR on the documentation to describe the new feature. How does that sound? Any objections from the others?

[timbussmann]

Looking at the usage from handlers/behaviors I think we should not move ahead with this in its current state. The API usage of the interface is too different from the regular transactional session usage as within the pipeline both Open and Commit are managed by the pipeline while outside those are managed by the user. I feel this creates a significant inconsistency in how this API is used and understood. It requires too much implicit knowledge about the session to be able to correctly use it and to understand what it does. I would prefer to take a step back and review the original API design with the goal of making this also usable within the pipeline first.

[danielmarbach]

Looking at the usage from handlers/behaviors I think we should not move ahead with this in its current state. The API usage of the interface is too different from the regular transactional session usage as within the pipeline both Open and Commit are managed by the pipeline while outside those are managed by the user. I feel this creates a significant inconsistency in how this API is used and understood. It requires too much implicit knowledge about the session to be able to correctly use it and to understand what it does. I would prefer to take a step back and review the original API design with the goal of making this also usable within the pipeline first.

I'd be ok with that. This was just a spike to get us thinking a bit. If we prefer to split the interface to segregate the session part from the commit part, that would certainly work. I'm personally not too concerned about this though because there are things like EntityFrameworkContext, SqlConnection etc. that also require knowledge about when it is appropriate to call a certain method like SaveChangesAsync and when not.

As an alternative, I could also push an update to the branch to segregate the interface in case anyone has good naming proposals. That being said, I'm happy to let it go inf that is the consensus.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[danielmarbach]

The state handling could be removed if we are willing to make sure the pipeline session behaves a bit different than the other sessions. Example

    sealed class AttachInvokeHandlerContextBehavior : IBehavior<IInvokeHandlerContext, IInvokeHandlerContext>
    {
        public async Task Invoke(IInvokeHandlerContext context, Func<IInvokeHandlerContext, Task> next)
        {
            var transactionalSession = context.Builder.GetRequiredService<ITransactionalSession>();
            // Always opening the transactional session to attach the current pipeline context
            // The pipeline aware session can be opened multiple times
            await transactionalSession.Open(new PipelineAwareSessionOptions(context), context.CancellationToken).ConfigureAwait(false);

            await next(context).ConfigureAwait(false);

            // the pipeline aware transactional session doesnt have to be committed since that is done
            // implicitly by the load handler connector managed the synchronized storage interaction
        }
    }

[danielmarbach]

I still think this is a great idea but I'm closing it for now. We can always pick this up at a later stage when we have looked more into the ramifications of this potentially pushing customers to use transactional session as the IBus of NServiceBus.