mahara / Castle.Facilities.NHibernateIntegration

Castle.Facilities.NHibernateIntegration
https://www.castleproject.org/
Apache License 2.0
9 stars 12 forks source link

Allow the facility to work with async methods and TPL/multithreading #1

Closed mahara closed 5 years ago

mahara commented 8 years ago

CallContext

LogicalCallContext

AsyncLocal<T>

Liwoj commented 8 years ago

Sorry for late reply

OK. So, I'm not an expert in async/threading stuffs nor has enough knowledge about them, but I take it that LogicalCallContext spans multiple threads, but your expected case is to allow only a single ISession or IStatelessSession within/across a single async logical call, that will not span multiple threads. Is that correct?

I'm not expert either ;)

LogicalCallContext (part of CallContext which is part of ExecutionContext) flows through async points (source). Await keyword is just one of them. Others are for example Task.Run, ThreadPool.QueueUserWorkItem, Delegate.BeginInvoke etc.

And that's the problem. Storing session store in LogicalCallContext makes it work in (simple ...see note below) async\await scenarios but at the same time makes single instance available to many threads (and new .NET 4.5 copy-on-write behavior doesn't change anything on that because of the way session store works). Now because session store and NH session are not thread safe, it just breaks NH integration facility for ANY multi threading scenario.

NOTE: By simple async\await i mean scenarios not involving more than one thread. Async\await is very different from threading - you can have async chain where every part is executed on the same thread (UI thread for example). And that's the only scenario where storing session store in LCC works...

I'm not that interested in async\await TBH. But i'm sure making it work (not in 100% cases) at the cost of breaking multi threading altogether is just bad idea.

Also, AsyncLocal seems to be new to .NET 4.6, which not available to .NET 4.5 (SP2) or below. So, any suggestion how this should be made to be work in .NET 4.5 (at least)?

I'm not sure if AsyncLocal is correct solution here. Just pointed out it might be and it is now used on other NHIF fork. I would definitely do some test before using it to make sure it doesn't break NHIF in multi threading scenarios. If it does, then there is probably not clean solution for this and i would suggest not to use async\await together with NHIF OR make sure no session is kept open on async point transitions....

mahara commented 8 years ago

OK, so I still don't get it on how to make this works properly. I can't really see any feasible trade-off to make both scenarios working properly, other than providing a way to set different custom session stores for different scenarios used.

So for now I've made some changes: