1

I created a library that handles database access. I recently added transaction handling; however, I came across a small issue. To outline this, I wrote this sample for demonstration purposes:

class Program
{
    static void Main(string[] args)
    {
        String data = null;
        DoAction(ref data, () =>
        {
            Console.WriteLine(data);
        });
        Console.ReadLine();
    }

    private static void DoAction(ref String data, Action action)
    {
        if (data == null)
            data = "Initialized Data";
        action();
    }
}

I get "Access to modified closure" underline on the following code line for the 'data' variable:

Console.WriteLine(data);

I understand that the modification of the ref data variable can cause issues (e.g. when running foreach loops). However, in the following case, I don't see this to happen.

Here is another version with a loop changing the variable further - the output is as expected:

class Program
{
    static void Main(string[] args)
    {
        String data = null;
        for (var i = 0; i < 10; i++)
            DoAction(ref data, () =>
            {
                Console.WriteLine(data);
            });
        Console.ReadLine();
    }

    private static void DoAction(ref String data, Action action)
    {
        if (data == null)
            data = "Initialized Data";
        else
            data += "|";

        action();
    }
}

ReSharper offers me to create a local variable, but I explicitly want to use the created string from the DoAction() method. If I would accept ReSharpers approach, it actually would break the code. Is there any other way to solve this problem? I'd like to use this Action approach, but I don't want ReSharper to complain about it either (and possibly not disable ReSharpers inspection).

Any suggestions?

Infinity Challenger
  • 1,090
  • 10
  • 19
Alexander Fuchs
  • 147
  • 10
  • I looked also at: [Access to Modified Closure (2)](http://stackoverflow.com/questions/304258/access-to-modified-closure-2) which contains a link to ReSharper documentation in the original posts comment. – Alexander Fuchs Aug 15 '15 at 10:16

2 Answers2

4

I would suggest avoid using a ref parameter for this in the first place - it seems needlessly complicated to me. I'd rewrite DoAction as:

static string DoAction(string data, Action<string> action)
{
    data = data == null ? "Initialized Data" : data + "|";
    action(data);
    return data;
}

Then you can have:

data = DoAction(data, Console.WriteLine);

or if you want to use a lambda expression:

data = DoAction(data, txt => Console.WriteLine(txt));

You can make DoAction a void method if you don't actually need the result afterwards. (It's not clear why you need the result to be returned and a delegate to execute in DoAction, but presumably that makes more sense in your wider context.)

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • I understand - and I have done the same thing. However, I would have twice a suitable variable for Console.WriteLine(string). data as well as txt. It could lead to confusion for other team members. – Alexander Fuchs Aug 15 '15 at 10:21
  • 1
    @AlexanderFuchs: Well it's a somewhat confusing method to start with - why is `DoAction` responsible for computing a new value *and* executing an action? It's hard to tell what the best solution is without knowing more about what you're really trying to achieve, but using a `ref` parameter *and* a closure seems like a bad idea to me. – Jon Skeet Aug 15 '15 at 10:23
  • Sorry, I didnt understand the solution before the Lambda. This looks nice! I will try it – Alexander Fuchs Aug 15 '15 at 10:23
  • The idea was to have sql statements execute with transaction. The DoAction method would create a transaction and use it for all queries in the Lambda block. Don't know if that makes sense. – Alexander Fuchs Aug 15 '15 at 10:26
  • @AlexanderFuchs: Yes, that makes sense, but then it's not clear why you need to pass `data` into the method rather than *only* referring to it in the closure. – Jon Skeet Aug 15 '15 at 10:27
  • I might have an existing Transaction already and only create a new Transaction if the 'data' is null. This is the reason why I would like to pass it. – Alexander Fuchs Aug 15 '15 at 10:29
  • @AlexanderFuchs: Hmm. It feels like you might be better encapsulating this in a separate transaction-wrapping class, to be honest. But even if you want to stick to your rough current design, I would go with the option here rather than using `ref`. – Jon Skeet Aug 15 '15 at 10:38
  • I decided to go with the Lambda expression because it allows for multiple statements and it seems to be the cleanest solution. Each person just needs to pay attention what variable is referenced inside the Lambda block. – Alexander Fuchs Aug 16 '15 at 17:32
  • @AlexanderFuchs, also look at threadLocalStorage if you need to track if you already have a transaction running and can't use nested transactions for some reason. – Ian Ringrose Aug 20 '15 at 14:20
2

In case you feel certain that the warning is not appropriate, there is the InstantHandleAttribute which is documented as:

Tells code analysis engine if the parameter is completely handled when the invoked method is on stack. If the parameter is a delegate, indicates that delegate is executed while the method is executed. If the parameter is an enumerable, indicates that it is enumerated while the method is executed.

I think is exactly what you want.

You can get the attribute from the JetBrains.Annotations package or alternatively as copy-paste from ReSharper options.

Matthias
  • 15,919
  • 5
  • 39
  • 84
  • I will give this a try – Alexander Fuchs Aug 16 '15 at 07:24
  • Dear god, I have searched for a long time for this and randomly stumbled upon this solution. I had the same issue, having an Action that gets executed synchronously in the method and then is done, but ReSharper kept telling me the modified closure warning. This is the solution, no workaround needed. – Wolfsblvt Jul 26 '17 at 10:02