Fix https://github.com/NuGet/WebBackgrounder/issues/7 - unhandled Except... #8

Closed
wants to merge 2 commits into from

1 participant

@TimLovellSmith
NuGet member

...ions from executing query in RunInTransaction (from miscellaneous failures such as SqlException) fail to restore _context to a valid state that allows further calls to RunInTransaction. (Fix #7)

@TimLovellSmith TimLovellSmith Fix #7 - unhandled Exceptions from executing query in RunInTransactio…
…n (from miscellaneous failures such as SqlException) fail to restore _context to a valid state that allows further calls to RunInTransaction.
681ae37
@Haacked Haacked and 1 other commented on an outdated diff Feb 22, 2013
...kgrounder.EntityFramework/EntityWorkItemRepository.cs
@@ -23,13 +23,20 @@ public void RunInTransaction(Action query)
// For some reason, I get different behavior when I use this
// instead of _context.Database.Connection. This works, that doesn't. :(
((IObjectContextAdapter)_context).ObjectContext.Connection.Open();
- query();
- transaction.Complete();
+ try
+ {
+ query();
+ transaction.Complete();
+ transaction.Dispose();
+ }
+ finally
+ {
+ // REVIEW: Make sure this is really needed. I kept running into
+ // exceptions when I didn't do this, but I may be doing it wrong. -Phil 10/17/2011
+ _context.Dispose();
+ _context = _contextThunk();
@Haacked
NuGet member
Haacked added a note Feb 22, 2013

This line makes no sense to me.

If you remove this line, then you can probably make _context declared as readonly. Then you don't need to do the whole local assignment thing we do in the Dispose method.

@Haacked
NuGet member
Haacked added a note Feb 22, 2013

Nevermind. I see the point of this now. 📦

But it makes me think it's possible that _context could be null if the contextThunk returned null. So we should probably guard against that just to be safe, right?

@TimLovellSmith
NuGet member

I think it would be bad if _context ever returns null - it's a factory function, obviously if it fails it should throw, like a constructor would. Also we don't guard against _context being null anywhere except the Dispose() method currently. But the irony of that is even Dispose() doesn't set it to null.

@Haacked
NuGet member
Haacked added a note Feb 22, 2013

I agree that _contextThunk should never return null. But thanks to the lack of support in the C# type system, there's no guarantee it won't. Yes, it'd be a programming error if that happened, but we should fail fast and make it clear that's why. So maybe on each call to it either Debug.Assert it's not null OR check the result of the call to _contextThunk and throw an exception if it's null.

Also, i noticed we forgot to check the arguments of the constructor. So it's possible that _contextThunk is null in the ctor resulting in a NullReferenceException. It'd be more helpful if that were an ArgumentNullException. BTW, I'm keenly aware that this is probably all my fault. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@TimLovellSmith TimLovellSmith +Unit tests, add null checking tests, ensure EntityWorkItemRepository…
…._context can never be assigned null, and simplify the Dispose() implementation to rely on transitivity for correctness.
91a0eb6
@TimLovellSmith
NuGet member

I've decided it's really weird that we also ensure the database connection is open as part of the transaction, so I've tried to shuffle it around a little, and simplify Dispose() given the new invariant that _context is never null.

@TimLovellSmith
NuGet member

Any more thoughts? :)

@TimLovellSmith
NuGet member

I now think that this might be the wrong fix. Because there's still a weird code path where
a) we dispose the old context
b) ensure context fails to get us a new context.
Withdrawing PR.

@xavierdecoster xavierdecoster deleted the Fix_#7 branch Nov 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment