Why implicit transactions at Session creation?

Topics: Core, Customizing Orchard, General, Troubleshooting, Writing modules
Apr 18, 2013 at 2:09 PM
I was working on doing a massive (2600 post) Wordpress import and kept running into transaction timeouts.

I realized, after much review of the module code (it wasn't quite there) that there is an overall "implicit" transaction that is created with every new session (Line 31, SessionLocator.cs).

I think that creating an implicit transaction does few things that I would consider less than optimal.

1) It puts a clock on any use of a session object to less than the transaction time limit, which in the case of imports is very bad :-(.

2) It creates a scenario where the developer may demand a transaction and use it; "believing" there work has committed, only to find out they are inside another transaction scope (the session scope) and gets rolled back due to an error somewhere else.

3) Its a needless performance hit to the 70%~90% of orchard activity which is read only access to view content by a website user.

4) there is no supported way to run outside of a transaction if they deem that they need to.

5) Nested Transactions of the ReadCommitted scoping can cause lots of deadlock and performance issues, as have been noted by other comments.

I would propose that transactions be on "demand" only... As developers and module writers, we should be smart enough to know when to use transactions (during writes to storage) and when not to.

Placing an implicit transaction seems to create more overhead and risk then it provides in protections (if you ask me)..

Thoughts, comments, omissions, concerns? I know I am writing this rather, matter of fact, but I just laying all my concerns on the table to open up a dialog on the subject :-).

Josh
Apr 20, 2013 at 9:47 PM
Edited Apr 24, 2013 at 11:39 PM
  1. I think the old Transaction scope for Orchard is going away very shortly, and being replaced with Nhibernate's own transaction layer, which may improve things? Can anyone comment on how that will effect things, and if im talking nonsense and dreamt it up.
  2. Yes I've run into that a few times, also when there is no space in the database and it says success, only after running to the logs do you notice what is really happening.
  3. Ive not needed to improve the performance of any of my sites so not really observed this.
  4. There is sort of. You can write a suppression to the transaction. There are a few examples of people needing to talk to other databases and suppress the transaction for the duration of that work. This works fine and we have been using that solution on sites for over a year now. Its not ideal granted.
  5. Ive not experienced this myself so no comment.
rest.... Do I like it being on by default. In places. In a content type the data is potentially handled by a range of different modules. It would be hard to coordinate with the others for the safety of the data and when it might be important to fail seeing the saving is done out of sight and out of our control. Maybe a content type handler could be implemented to scope the read, write around the phase. For a repository, often then not all i need is a quick data insertion and manipulation so a transaction would slow that down potentially therefore more control might be nice.

Overall I have changed the way I work in the modules. I have shorter bits of work (actions), longer actions are done in shorter chunks, more action is done on the client side etc.
Trying to get the content manager to behave properly in SignalR was a pain due to the lifespan, and it were out of session as it were. Better people than I manage to solve those things. Without the complexity of the session then it wouldn't have been so hard to implement and make this work.

Debugging is still a pain, but it tends to be far from a easy thing like finding why something might not be showing when it should. Especially when its disappeared after working for months. It hardly seems appropriate for shape tracing for things like content editors which you expect to just work. I also have a collection of bugs that I really should add to the Issue tracker. Ah time, why isn't there more in the day.
Coordinator
Apr 22, 2013 at 3:36 AM
Please provide evidence of #3.
May 21, 2013 at 2:48 PM
I'll have to spin up my test environment again (with 3 laptops hitting a 4th)... But as I remember, it was a couple of percent difference when I went into the core and turned transactions off then with them on... Granted once you add output caching its not as big a deal because the output caching takes a HUGE chunk out of any performance concerns..

But fundamentally why slow down Orchards primary workload (reading data) by wrapping everything in a transaction.. Its overhead for the DB and the Managed code that's just not necessary.
Coordinator
May 21, 2013 at 5:57 PM
Transactions are necessary because Orchard uses them as a feature. If a part succeeds to update its content, then a subsequent one fails, the content item should not be save at all. Thus transactions. Also, in NHibernate, to be able to use the second level cache you need transactions.
May 21, 2013 at 6:00 PM
I agree 100% regarding anything that is a write.. So 95% of the admin screens, and I suppose a few modules like comments... But If the site spends most of its time rendering content that's where it feels wanky to me... However, I didn't know about NHibernate needing it for caching.
May 22, 2013 at 12:08 PM
Edited May 22, 2013 at 12:14 PM
Site Admins should have the choice either implement a fully 'secured' site with transactions everywhere, either chose a 'higly faster' one taking the risk of some inconsistencies (and with read uncommitted enabled).
This behaviour would necessitate some new writting of modules were some transactions will depend on the previous setting and some others would persist whatever is the setting, mandatory transactions. An Attribute could simplify this.
Coordinator
May 22, 2013 at 5:47 PM
Again, Orchard is dependent on this feature, and Nhibernate requires it too.

But feel free do fork Orchard and show us how little work it takes and how much performance gain you get. Might convince everyone.
May 22, 2013 at 6:40 PM
Edited May 22, 2013 at 6:41 PM
Ok, received.
Forking Orchard with a serious proposal is something so crazy I actually don't even think of :)
Coordinator
May 26, 2013 at 7:46 AM
A few things...
  • Forking Orchard is not crazy in any way. This is how pull requests are prepared. It's standard procedure when working on patches.
  • It's a common misconception that transactions are only useful for writes.
  • How do you know in advance whether a given request will result in read-only operations?
  • It's a common misconception that transactions adversely affect performance.
The last point is why Sébastien is suggesting that you show us the evidence of a performance gain. The assertion that such a change would make it "highly faster" is not grounded in reality until you've forked the code and demonstrated it.