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 the facility to work with async methods and TPL/multithreading #1

Closed
mahara opened this issue Apr 2, 2016 · 2 comments
Closed

Comments

@mahara
Copy link
Owner

mahara commented Apr 2, 2016

CallContext

  • Doesn't work well with async methods, especially when a single async method's call chain could be run on multiple different threads internally. Thus it can cause multiple ISessions created on a single async method's call chain, and could break the assumption that only a single ITransaction (created from ISession.BeginTransaction()) should be used within a single async method's call chain (atomicity of a transaction)
  • Works well with TPL/multithreading, as a new ISession being created for each different thread

LogicalCallContext

  • Works with simple (linear) async methods; might not work with complex async methods which utilize TPL/multithreading
  • Doesn't work as expected with TPL/multithreading, as a same new ISession being used by multiple threads concurrently (ISession doesn't support (concurrent) multithreading))

AsyncLocal<T>

  • Requires .NET 4.6, not available on .NET 4.5 or below
  • Unknown/untested yet with both scenarios
mahara referenced this issue Apr 2, 2016
…nd CallContext.LogicalSetData() to allow the facility to work in async methods.
@Liwoj
Copy link

Liwoj commented Apr 9, 2016

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 mahara changed the title Allow the facility to work with async methods Allow the facility to work with async methods and TPL/multithreading Apr 9, 2016
@mahara
Copy link
Owner Author

mahara commented Apr 13, 2016

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:

  • I reverted the behavior of CallContextSessionStore to previous behavior; to use CallContext instead of LogicalCallContext.
  • I introduced a new fluent API NHibernateFacility.SessionStore<T>() in order to allow a custom session store to be set fluently (and programmatically).
  • I added LogicalCallContextSessionStore which use LogicalCallContext for storing the ISessionStore data for allowing (simple and linear) async methods to work properly.
  • I defaulted the facility's session store to LogicalCallContextSessionStore instead of CallContextSessionStore. The default session store can be changed through XML file configuration and programmatically (using NHibernateFacility.SessionStore<T>()).

@mahara mahara pinned this issue May 13, 2019
@mahara mahara closed this as completed May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants