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

Allow access the NHibernate SynchronizedStorageSession with the TransactionalSession without having to open it first #1019

Open
hugovlc opened this issue Jul 19, 2023 · 12 comments

Comments

@hugovlc
Copy link

hugovlc commented Jul 19, 2023

Describe the suggested improvement

Currently with the TransactionalSession we can open a transaction and then have access to the SyncronizedStorageSession where we can get the ISession.

In a scenario with dependency Injection where we might need it or not, in order to have access to the ISession we currently need to open the transaction. We should be allowed to have access to the SyncronizedStorageSession even if we don't open a TransactionSession.

I understand this was implemented like this, I guess the feature wants to prevent NHibernate transactions to be created outside of the scope of the TransactionalSession context, but this should be the responsibility of the developer.

Don´t know if this should be created here or in the TransactionalSession repository.

Additional Context

No response

@hugovlc hugovlc changed the title Allow access the NHibernate Session with the TransactionalSession without having to open it first Allow access the NHibernate SyncronizedStorageSession with the TransactionalSession without having to open it first Jul 19, 2023
@hugovlc hugovlc changed the title Allow access the NHibernate SyncronizedStorageSession with the TransactionalSession without having to open it first Allow access the NHibernate SynchronizedStorageSession with the TransactionalSession without having to open it first Jul 20, 2023
@ramonsmits
Copy link
Member

@hugovlc You want to have access to a NHibernate session factory that is used by the transactional session and also the incoming message pipeline?

@hugovlc
Copy link
Author

hugovlc commented Jul 20, 2023

Thank you for the reply.

I want to have the ISession. What happens is that the session is built when we open a Transaction with the TransactionalSession.
Would be nice if the ISession could be opened first instead. This also would require changing the NServiceBus.TransactionalSession package.
This is problematic, we use dependency injection (inject the ISession to multiple services), but the issue is that I have to open a transaction (TransactionalSession) first to register my ISession. I saw the code and the a OutboxTransaction is created (and then the ISession is created from the session factory and then immediately a NHibernate transaction starts).

Is related to the SynchronizedStorageSession. I think we should have the option to open it without a NServicebus transaction.
I did some changes in the repositories and made it work. Was not hard. But I don't wanna maintain this code to be honest when could be easily implemented.

Basically I changed the NServiceBus.TransactionalSession to have the possibility to open the SynchronizedStorageSession and then changed the NServiceBus.NHibernate NHibernateLocalOutboxTransaction and OutboxPersister to pass the ISession created previously. I did not implemented for sagas or TrsanctionScope.

@ramonsmits
Copy link
Member

@hugovlc Can I summarize it to that you would like to the ability to:

  1. create storage context to obtain the storage-specific session
  2. create a transactional session based on a existing storage specific session

May I ask why you couldn't just create the transactional session immediately? Why is that a bad approach for your application?

@hugovlc
Copy link
Author

hugovlc commented Jul 21, 2023

Is bad practice in general having the data in a Transactional mode if I only want to query data.
I inject the ISession to several services and with DI I always would have to create a transaction in order to do simple things.
Then is the danger of creating transient errors if the data is locked, lag and slowness.

So having access SyncronizedStorageSession (with another method in the ITransactionSession interface) , and then when I open a OutboxTransaction, that session could be passed if SyncronizedStorageSession is already opened. if not use the current default behavior.

@ramonsmits
Copy link
Member

@hugovlc what kind of queries on what data would you like to make? I assume this is data which not part of a saga context?

@ramonsmits
Copy link
Member

ramonsmits commented Jul 21, 2023

@hugovlc Side note:

I assume you are using connection pooling and when you do you always need to specify a transaction mode. Even when not using transactions you should for example do:

SqlConnection.BeginTransaction(IsolationLevel.ReadUncommitted).Dispose();

Not doing so can result in strange transaction isolation issues as the isolation level is never reset when a connection is returned into the connection pool as its assumed that a new consumer will always set its required isolation level.

@hugovlc
Copy link
Author

hugovlc commented Jul 22, 2023

I understand that. The issue is that in any method i call, for example a rest api call, i have ro open the nservicebus transaction and then change all my calls that does not uses nservicebus im order to close the transaction. Again i am using DI and cannot cherry pick a different ISession for each case. The ISession should be available as a extra option in the TransactionalSession. I already forked both the Nhibernate and TransactionalSession repos and the change was very easy. But i don't wanna maintenant the code. I think this is logical.

@ramonsmits
Copy link
Member

@hugovlc I'm not 100% sure what you are asking for. I thought its only about resolving an ISession but maybe it's better if you push your changes.

You state that you want to have access to the ISession for queries but I'm stating that you always need to create a transaction. Even with ISession you would need to do

using (ITransaction transaction = session.BeginTransaction(IsolationLevel.ReadCommitted))
{
	// Do your queries here
}

From what I understand from you is that you would then later pass this session into the transaction session Open. This could conflict if you would later want to use that same session as-is with a potential lower isolation level for the transactional session as I'm reasonably sure you don't want to use that isolation level when doing modifications.

As sessions are cheap to create I think it would be ok if the transactional session creates its own session but I think I'm misunderstanding something from your responses.

  1. Why is it important that all share the same session transaction? As sessions are cheap would it be ok if you could only obtain an ISession?
  2. What isolation level requirements do you have for your queries?

As you state you already made some changes could you please share your changes including maybe a small demo project? If these are good changes we can consider merging them in a future maintenance release or use them as inspiration for a similar API change.

@hugovlc
Copy link
Author

hugovlc commented Jul 24, 2023

Hi. Sorry to say you are wrong. We don't need to create a Transaction every time we query something.
Also NHibernate has something called implicit Transactions when for example we do Flush.

So let's say I Have a controller and I have a Service Layer to do something.
So imagine in the controller I have 2 endpoints, One to return data, another to save data.

I have a UnitOfWork to handle the DB operations (Transactions, interceptions, etc)
So in the controller will try to resolve my UnitOfWork and the service, then the service the ISession.

The ISession needs to be accessible, So in my service registration I have to register an instance, but first I need to resolve the ITransactionSession service, then open a transaction.

So in EVERY METHOD that I do not commit anything I need to refactor it.. Is just not doable.
I will push my changes soon.

@ramonsmits
Copy link
Member

ramonsmits commented Jul 24, 2023

Hi. Sorry to say you are wrong. We don't need to create a Transaction every time we query something.

@hugovlc Actually, I'm not wrong. That is the only way to state your required isolation level and not accidentally query using the wrong previous connection cached isolation level. So unless you are not using connection pooling as mentioned earlier you really need to do this.

An isolation level has connection-wide scope, and once set for a connection with the SET TRANSACTION ISOLATION LEVEL statement, it remains in effect until the connection is closed or another isolation level is set. When a connection is closed and returned to the pool, the isolation level from the last SET TRANSACTION ISOLATION LEVEL statement is retained.

Source: https://learn.microsoft.com/en-us/dotnet/framework/data/adonet/sql/snapshot-isolation-in-sql-server?redirectedfrom=MSDN#managing-concurrency-with-isolation-levels

To reset the isolation level to the default (READ COMMITTED), execute the Transact-SQL SET TRANSACTION ISOLATION LEVEL READ COMMITTED statement, or call SqlConnection.BeginTransaction followed immediately by SqlTransaction.Commit.

Source: https://learn.microsoft.com/en-us/dotnet/api/microsoft.data.sqlclient.sqlconnection.begintransaction?view=sqlclient-dotnet-core-2.1

Only when ALL database operations are based on the same isolation level (for example, ReadCommitted) you don't need this.

@ramonsmits
Copy link
Member

I will push my changes soon.

👍🏼

@hugovlc
Copy link
Author

hugovlc commented Jul 25, 2023

I think you miss understood me.
Of course we always need a transaction to do any kind of DB operations, what I meant is that I don´t implicit have to do it for NHibenate. I use implicit transactions for querying data. I know is not best practice but it works for us.

Also btw we use Oracle as business DB with SqlServer Transport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants