NHibernate Saga persister does not work with Oracle

[johnsimons]

The issue is that the Oracle driver does not support both TransactionScope and NH transaction, see http://stackoverflow.com/q/6870901/90882 for more info.
So we now checking if there is currently an active ambient transaction and if there is we do not create a NH transaction.
We also disable AutoFlush and control Flush manually.

Still to do:

• fix broken acceptance tests
• code review

[johnsimons]

Oracle limits the name of tables to maximum 30 characters, this makes it hard to get all acceptance tests to pass.

[DennisNerush]

Is there a way to use your solution right now?

[johnsimons]

@johannesg code review please?

[johannesg]

Well.

1. Remember why the nh transaction is there in the first place. It there to
   prevent connection leaks that can occur if an exception is thrown and NH
   fails to clean up properly. I have said that it might be ignored in the
   UnitOfWork class because the UoW class explicitly manages the connection
   object, although it has to be tested thoroughly. Interestingly, this patch
   seems to change every other place except the UoW, and none of these seems
   to manage their own connections. Also, the subscription storage will be
   unaffected by this change, since it is never using a tx scope in the first
   place. I don't know about the other ones though.
2. You have changed Flushmode to Never. This is a behaviour change and
   needs to be documented. Some users may rely on flushmode being set to auto.
   Also, IMHO, I don't really see the point of changing it.

My 2 cents.

2014-03-25 8:53 GMT+01:00 John Simons notifications@github.com:

@johannesg https://github.com/johannesg code review please?

Reply to this email directly or view it on GitHubhttps://github.com//pull/28#issuecomment-38538228
.

[johnsimons]

@johannesg thanks a lot for the code review, very much appreciated.

Regarding your points:

• I have reverted the SubscriptionStorage to use the NH Transaction as you pointed out there was no point of doing the check.
• The UoW was modified to skip NH Transaction, see https://github.com/Particular/NServiceBus.NHibernate/pull/28/files#diff-64db7208eb23a5918f34de5eb5e35aedR100
• About managing our own connections, @johannesg and @andreasohlund I may be completely wrong here so please correct me, my understanding about all the saga debacle and the fix we put to address NHibernate + distributed transaction on different thread #1 is that on a rollback when DTC is involved connections, NH closes the connection on a different thread and that causes issues, so we fixed the UnitOfWorkManager with this commit, but from reviewing the code for TimeoutStorage, GatewayDeduplication and GatewayPersister wouldn't all these NHibernate implementations need the same fix?
• About the FlushMode, so are you saying that I should not manually Flush and instead let NH handle it?

[johannesg]

The UoW was modified to skip NH Transaction, see https://github.com/Particular/NServiceBus.NHibernate/pull/28/files#diff-64db7208eb23a5918f34de5eb5e35aedR100

Oh. My bad.

but from reviewing the code for TimeoutStorage, GatewayDeduplication and GatewayPersister wouldn't all these NHibernate implementations need the same fix?

Yes, if they use transaction scope. However, maybe these can be designed to not use DTC in the first place? Just a thought...

About the FlushMode, so are you saying that I should not manually Flush and instead let NH handle it?

I would say that they are not related. You should always do a flush, either using session.Flush() or using tx.Commit() (which automatically does a flush if flushmode is "Auto" or "Commit").

However, flushmode "Auto" means that NH might call Flush on multiple occasions. For example:

var foo = session.Get<Foo>();
foo.SomeValue = 1;

// This will trigger a Flush if flushmode == Auto
var foos = session.QueryOver<Foo>().List();

See here: http://weblogs.asp.net/ricardoperes/archive/2013/01/25/nhibernate-pitfalls-flush-mode.aspx

So: while FlushMode.Never might give a small performance boost in some cases, it is not really related to this fix.

Hope that helps.

[johnsimons]

However, maybe these can be designed to not use DTC in the first place? Just a thought...

Yes, @andreasohlund started #21

[johnsimons]

@andreasohlund @johannesg I'm now managing the connections explicitly for timeouts and gateway.
Another quick review?

This should be it hopefully.

[johannesg]

Looks good to me.

2014-03-27 8:44 GMT+01:00 John Simons notifications@github.com:

@andreasohlund https://github.com/andreasohlund @johannesghttps://github.com/johannesgI'm now managing the connections explicitly for timeouts and gateway.
Another quick review?

This should be it hopefully.

Reply to this email directly or view it on GitHubhttps://github.com//pull/28#issuecomment-38776208
.

[andreasohlund]

Looks good , 👍