8

I have the following code:

public void DequeueRecipe(AuthIdentity identity, params Guid[] recipeIds)
{
   using (var session = GetSession())
   {
      var recipes = (from r in recipeIds select new Models.Recipes {RecipeId = r}).ToArray();

      var dbRecipes = session.QueryOver<Models.QueuedRecipes>()
         .Where(Expression.Eq("UserId", identity.UserId))
         .Where(Expression.InG("Recipe", recipes))
         .List<Models.QueuedRecipes>();

      using (ITransaction transaction = session.BeginTransaction())
      {
         dbRecipes.ForEach(r => session.Delete(r)); // <-- Warning
         transaction.Commit();
      }
   }
}

reSharper is giving me the warning:

Access to disposed closure

On the line:

dbRecipes.ForEach(r => session.Delete(r));

(The session variable is underlined as the culprit).

While it's true the ForEach method takes a lamba expression which creates a closure around the variable session, I don't see a way where it would possibly be disposed when this code is executed. Perhaps reSharper thinks ForEach might execute some sort of task in parallel, or save that Action<> for a later time, thus technically it might be disposed while the anonymous function is still reachable in memory.

Am I safe the ignore this warning? Is there a way I can reformat my code to prevent this warning from appearing? Is there indeed a danger presented by this code?

I'm used to reSharper being smarter than me, so I'd like to understand exactly what's going on.

Mike Christensen
  • 88,082
  • 50
  • 208
  • 326
  • unrelated: isn't the transaction begin/commit the responsibility of the Delete()?. Is it possible/correct to perform a Delete() outside of a transaction (assuming it has multiple actions [tables])? – Mitch Wheat Jan 11 '14 at 00:56
  • @MitchWheat - Agreed, I can remove the transaction since it will all be committed when the session is disposed. I've been reworking my session code, so that's a remnant from my earlier architecture. – Mike Christensen Jan 11 '14 at 01:01
  • I've also gotten this error when using this Batch code snippet for grouping an IEnumerable into batches of size x: https://stackoverflow.com/a/44505349/1040437 My boss saw the warning in Resharper and flipped out. But as best as I can tell, this is entirely safe code. – John Zabroski Mar 05 '19 at 16:25

1 Answers1

11

Because session is wrapped in a using statement, and the LINQ execution could be defered until it is enumerated.

Resharper is warning that this could result in an exception, because by the time dbRecipes is enumerated, the session could have have been disposed.

I'll be honest: I'm not sure the above code could ever fail in the way warned about.

Mitch Wheat
  • 295,962
  • 43
  • 465
  • 541
  • 2
    `ForEach` should immediately enumerate `dbRecipes`, right? So nothing should be deferred. – Mike Christensen Jan 11 '14 at 01:04
  • Wouldn't you consider that a false warning on ReSharper's part then? Could `session` really be disposed in that code? I've never seen my code behave that way. From my understand of the `using` statement ReSharper is just wrong. – evanmcdonnal Jan 11 '14 at 01:04
  • 1
    what happens if you add a .ToList() to evaluate dbrecipes? – Mitch Wheat Jan 11 '14 at 01:08
  • 1
    @MitchWheat - The warning goes away if I do that. So reSharper is smart enough to know that the list is definitely enumerated at that time, but isn't smart enough to see that `dbRecipes` is a `Guid[]` and thus enumeration cannot be deferred anyway. – Mike Christensen Jan 11 '14 at 01:11
  • I think it is just erring on the side of caution. Plus does it know what .List() is? – Mitch Wheat Jan 11 '14 at 01:13
  • @MitchWheat - Oh nevermind, `dbRecipes` is a NHibernate object. reSharper probably knows of the .NET Linq extensions but doesn't know that .List` will enumerate the dataset. Interesting. – Mike Christensen Jan 11 '14 at 01:13
  • Interestingly enough, the warning *also* goes away if I change the line to: `dbRecipes.ForEach(session.Delete);` – Mike Christensen Jan 11 '14 at 01:13