ChallengeFind the bug fixes
In the following code there are two very subtle bug fixes that result from painful experience. Can you spot them?
public class Global: System.Web.HttpApplication { public static ISessionFactory SessionFactory = CreateSessionFactory(); protected static ISessionFactory CreateSessionFactory() { return new Configuration() .Configure(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "hibernate.cfg.xml")) .BuildSessionFactory(); } public static ISession CurrentSession { get{ return (ISession)HttpContext.Current.Items["current.session"]; } set { HttpContext.Current.Items["current.session"] = value; } } protected Global() { BeginRequest += delegate { CurrentSession = SessionFactory.OpenSession(); }; EndRequest += delegate { if(CurrentSession != null) CurrentSession.Dispose(); }; } }
Oh, and it isn’t a Null Reference Exception in the EndRequest event handler.
More posts in "Challenge" series:
- (01 Jul 2024) Efficient snapshotable state
- (13 Oct 2023) Fastest node selection metastable error state–answer
- (12 Oct 2023) Fastest node selection metastable error state
- (19 Sep 2023) Spot the bug
- (04 Jan 2023) what does this code print?
- (14 Dec 2022) What does this code print?
- (01 Jul 2022) Find the stack smash bug… – answer
- (30 Jun 2022) Find the stack smash bug…
- (03 Jun 2022) Spot the data corruption
- (06 May 2022) Spot the optimization–solution
- (05 May 2022) Spot the optimization
- (06 Apr 2022) Why is this code broken?
- (16 Dec 2021) Find the slow down–answer
- (15 Dec 2021) Find the slow down
- (03 Nov 2021) The code review bug that gives me nightmares–The fix
- (02 Nov 2021) The code review bug that gives me nightmares–the issue
- (01 Nov 2021) The code review bug that gives me nightmares
- (16 Jun 2021) Detecting livelihood in a distributed cluster
- (21 Apr 2020) Generate matching shard id–answer
- (20 Apr 2020) Generate matching shard id
- (02 Jan 2020) Spot the bug in the stream
- (28 Sep 2018) The loop that leaks–Answer
- (27 Sep 2018) The loop that leaks
- (03 Apr 2018) The invisible concurrency bug–Answer
- (02 Apr 2018) The invisible concurrency bug
- (31 Jan 2018) Find the bug in the fix–answer
- (30 Jan 2018) Find the bug in the fix
- (19 Jan 2017) What does this code do?
- (26 Jul 2016) The race condition in the TCP stack, answer
- (25 Jul 2016) The race condition in the TCP stack
- (28 Apr 2015) What is the meaning of this change?
- (26 Sep 2013) Spot the bug
- (27 May 2013) The problem of locking down tasks…
- (17 Oct 2011) Minimum number of round trips
- (23 Aug 2011) Recent Comments with Future Posts
- (02 Aug 2011) Modifying execution approaches
- (29 Apr 2011) Stop the leaks
- (23 Dec 2010) This code should never hit production
- (17 Dec 2010) Your own ThreadLocal
- (03 Dec 2010) Querying relative information with RavenDB
- (29 Jun 2010) Find the bug
- (23 Jun 2010) Dynamically dynamic
- (28 Apr 2010) What killed the application?
- (19 Mar 2010) What does this code do?
- (04 Mar 2010) Robust enumeration over external code
- (16 Feb 2010) Premature optimization, and all of that…
- (12 Feb 2010) Efficient querying
- (10 Feb 2010) Find the resource leak
- (21 Oct 2009) Can you spot the bug?
- (18 Oct 2009) Why is this wrong?
- (17 Oct 2009) Write the check in comment
- (15 Sep 2009) NH Prof Exporting Reports
- (02 Sep 2009) The lazy loaded inheritance many to one association OR/M conundrum
- (01 Sep 2009) Why isn’t select broken?
- (06 Aug 2009) Find the bug fixes
- (26 May 2009) Find the bug
- (14 May 2009) multi threaded test failure
- (11 May 2009) The regex that doesn’t match
- (24 Mar 2009) probability based selection
- (13 Mar 2009) C# Rewriting
- (18 Feb 2009) write a self extracting program
- (04 Sep 2008) Don't stop with the first DSL abstraction
- (02 Aug 2008) What is the problem?
- (28 Jul 2008) What does this code do?
- (26 Jul 2008) Find the bug fix
- (05 Jul 2008) Find the deadlock
- (03 Jul 2008) Find the bug
- (02 Jul 2008) What is wrong with this code
- (05 Jun 2008) why did the tests fail?
- (27 May 2008) Striving for better syntax
- (13 Apr 2008) calling generics without the generic type
- (12 Apr 2008) The directory tree
- (24 Mar 2008) Find the version
- (21 Jan 2008) Strongly typing weakly typed code
- (28 Jun 2007) Windsor Null Object Dependency Facility
Comments
HttpContext.Current is thread safe where a static member is not?
Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "hibernate.cfg.xml") looks like one...
It was probably the only "safe" way to get that path.
On of them might be using AppDomain.CurrentDomain.BaseDirectory to get the path of the executing assembly instead of Assembly.GetAssembly() or Assembly.GetEntyAssembly() and then figuring out the path based on their location.
I have hit a problem like that while unit testing a winforms app (entry< assembly was different of course, stupid me :-) ).
Why not just use CurrentSessionContext.Bind(..) and .Unbind(..) ?
You have changed this line:
.Configure(Path.GetFullPath(@".\hibernate.cfg.xml"))
to this:
.Configure(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "hibernate.cfg.xml"))
also the if(CurrentSession != null)
Putting things into constructor instead of application_start? don't know...
Ouch, ignore what I said. It is not a constructor.
One problem has to be a cast problem in the CurrentSession property getter:
return (ISession)HttpContext.Current.Items["current.session"];
This should be written as:
return HttpContext.Current.Items["current.session"] as ISession;
Your subtle bugs are enlightening and infuriating at the same time. Aside from the code to determine the path of the config file, nothing else seems like it could cause a problem - which means I'm missing a key understanding =(
@VirtualStaticVoid
You are wrapping your code in cotton wool. The code explicitly puts an ISession instance into the context at that key and therefore it is more than reasonable to expect an ISession instance to be retrieved from the same key.
Your solution also creates a nasty problem in that it hides the true error by causing a null ref exception at the first place the session is used rather than an invalid cast exception at the point the error could occur.
Why is "CreateSessionFactory" protected?
I've found that:
msdn.microsoft.com/.../....hostingenvironment.aspx
Is useful regarding reliably and outside of http context way to get the apps path.
(for reference)
I guess technically you could try and get the session before the BeginRequest has fired (or fired at all if it isn't a http thread).. and you assume a http context.. but those are things you could well accept if you dont spawn any other threads or try and use the session before the request as started (although perhaps ioc resolution happens before that).
I have no idea what the problems were, but I would assume it could be
creating the session factory in Application_start or on first use instead of static constructor
The Global method where the events get bound will never get called. It seems that it should be an instance ctor so that they get bound when the app starts.
Andy, there are multiple HttpApplication instances running on a website, they act almost as 'runners', and return to a pool where a set amount of them is kept alive, the ctor should run (wouldn't be subtle if it didnt I guess ;)).
One's got to be the Path.Combine, which is good practice any time you build up a path from multiple parts, since simple string concats can fail if slashes, etc. are/aren't provided when expected.
The second I'm not sure about, but I'd guess using AppDomain.CurrentDomain.BaseDirectory? I've seen a lot of code that just used "\" or similar, not taking into account that apps may be published in something besides a root directory.
When the protected Global is called ? I guess it is never called so the session is never created ?
public static ISessionFactory SessionFactory = CreateSessionFactory();
instead of a static constructor?
I think you might have changed CurrentSession.Close() into CurrentSession.Dispose().
It might have something to do with TransactionScope. With Session.Close(), NH may attempt to disconnect the connection while a transaction is still in progress... resulting in an ocassional exception.
I use CurrentSessionContext.Bind(..) and .Unbind(..) -- here's how:
zvolkov.com/.../...ASPNET-MVC-with-NHibernate.aspx
Off the top of my head, I'd guess that storing the CurrentSession in the request instead of the Session is a bug fix. One might assume that the session is safe because it'll get refreshed with each request. But then what happens with AJAX requests. Suddenly, your session drops from beneath you because of a concurrent dispatch.
The 2nd bug fix... just a guess.. but I think you changed NHinitialization from application_start (or init) into a static method. This is related to an issue in IIS7 where you can't initialize NH during application_start or init.
Diego & Johanas
I actually forgot about the BaseDirectory one, yes, that is certainly a common bug
Tuna,
Ctor is definitely a biggie, now tell me why.
Virtual,
Nope, it is not a bug. If you put something other than ISession there, it is your own damn fault.
Neal,
Look for something that isn't a problem. This isn't spotting t6he bug, this is spotting the FIX
Frank,
No reason, it can be private as well.
Andy,
Very close, but you haven't explained why it will never get called or why it should be instance ctor
Stephen,
YES!
You got one of the problems, the event wiring must happen in the ctor.
Application_Start, where I have seen a lot of people put them is going to cause problems when you have concurrent requests
Rafal & Rob,
Yes, but can you explain why?
Mike,
You really want to avoid putting the NH session in the ASP.Net session.
No, that is not it.
And sessions force serialization of requests anyway
Hendry,
You are correct about the change, but not about why I did it. I am not aware of the issue you brought up.
I think it's because there can be more than one Global instance.
Not sure if this is a problem in NH, but we had that problem in SharpArchitecture (or rather Fluent-NH) with IIS7 where NHibernate.Init() crashes when called in application_start.
code.google.com/.../detail
So they changed it to singleton-method call in each Application_BeginRequest:
NHibernateInitializer.Instance().InitializeNHibernateOnce(
Anyway, maybe you changed it because you want it invoked only once... And we can't guarantee application_start to be called once in a lifeatime of an application, particularly in the case of application-pool recycling.
Do you need to set SessionFactory to null when the application ends to avoid memory leaks?
Application_Start is not viable as many instances of HttpApplication is created, the later ones will not benefit from this. Init may be used for that purpose, instead of Application_Start.
protected void Global() is not a ctor and this made confused :)
The only thing I can think of is that perhaps the static init could silently fail, causing you to wonder why you don't have a session factory for subsequent requests.
Meh, scratch that, aparently static init will def~ happen in context of the first request.. maybe related to static field init ordering?
I think it is impossible to look at working code and guess what the error was in the old code that you have not seen. There are millions of possibilities.
For example, I think the code was here...
You took out the NotImplementedException in CreateSessionFactory()
:-)
Stephen,
I thought the same, the silent fail thing,it happened to me several times during dev, I had to catch the exception in the debugger to find the problem.
I think the "bug" that I can spot, is that you are creating a session factory each time there is a webrequest rather than caching up the session factory in a static reference and checking to see if ts null or not.
No, the session factory is only created once.
Stephen,
Silently failing is the key, but static init will NOT silently fail
Peter,
NotImplementedException is not subtle
Tuna,
Opps!
protected void Global is a typo ;-) Fixed
And yep, the event wiring must happen for every instance of http app.
OK, so.. is it that when running in classic mode (or IIS 6 and under), Application_Start will cause an exception only on the first request, but the next requests will all run and have no session factory (confusing to debug).. and the static initialization will throw every time a request tries to fire, thus making it easier to see what went wrong.
(Note that I think IIS7's default integrated mode now ensures the same for app_start).
Request isn't available for App_Start in IIS7
Also, you have a potential race condition under app_start if multiple requests come in. I'd assume however that IIS serializes/queues further requests until app_start completes. IF it doesn't though...none of the requests that came before App_Start completed will have the SessionFactory initialized.
ASP.NET uses a pool of HttpApplication instances, however Application_Start() method is called only on one of them. So when you bind events in the Application_Start() that the only one will have wired events and the others won't have wired events.
That is why you must use Global ctor.
You are using HttpContext.Current instead of [ThreadStatic] ISession property - this will prevent from various bugs that related to IHttpAsyncHandler.
Event handlers added in ctor will be fired before handlers registered via Global.asax (Application_BeginRequest method, Application_EndRequest and so on) and before handlers of modules. And, as I think, this is the only reason why you used this approach.
This is all I can see from this code at the moment.
And also you are using CurrentSession.Dispose() instead of CurrentSession.Close() because dispose method contains logic for logging of a session disposal. If you use Close method at the end of request information about session disposal will be logged only on run of GC. So the statistics will be wrong.
Stephen,
Yep!
I seeing one issue more if you will use asp.net mvc BeginRequest will be called when you getting .css, jpeg.... files. And the application will open one session for each image or style file loaded.
It would be smart to only open it for aspx and other file extensions which will use nhibernate.
@Niclas, this isn't specifically an mvc thing.. whatever requests you map IIS to handle (and with IIS7 this is even more so) will get sessions.. you would potentially filter by the handler type, not the extension which is unreliable.. an easier system which has other benefits is to move such static content onto another 'dump' domain, so you can cut the request / response down to an absolute min.
One thing I was wondering ayende, was why you chose to store the nhibernate session in in the http session, couldn't you just have an instance variable, and assign to it on begin request and dispose of it on end request. I'm sure IIS ensures that a given HttpApplication can only handle one request at a time (sounds obvious but async handlers could mean this isn't so).. is it purely so you can use the static helper property to find the session?
Stephen,
I am not putting it in the http sesson, I am putting it on the http request
Ah yes, completely misread the code.. I always assume Session when I see default property access with httpcontexts.
Apologies!
Comment preview