Pitfalls of Anonymous methods and capture

[Lars Fosdal 1792]

Code that looks correct, and appear to compile alright, but which doesn't execute well.
Can you spot the error? See my blog post for a link to a SCCE.

var
  Handler: THandlerClass;
  hType: THandlers;
begin
  Broker.Clear;
  for hType in [foo, bar]
  do begin
    case hType of
      foo: Handler := TFoo.Create;
      bar: Handler := TBar.Create;
    end;
    Broker.AddHandler(Handler.OnHandle);
  end;
end;

https://larsfosdal.blog/2019/02/08/delphi-pitfalls-of-anonymous-methods-and-capture/

[Attila Kovacs 629]

Hi Lars,

Ahm, where is Anonymous method in yor example? 

And I'm not sure about that for in over an enum.

I'm AFK but I would test it in first place.

[David Heffernan 2345]

If we have no knowledge of the types involved in this code then for sure we can't know what's wrong with it. 

[Uwe Raabe 2057]

I imagine what is happening and I assume it to be expected behavior, although I admit one has to know how variable capturing works to get it. It is even documented (Variable Binding Mechanism)

Quote

Note that variable capture captures variables--not values. If a variable's value changes after being captured by constructing an anonymous method, the value of the variable the anonymous method captured changes too, because they are the same variable with the same storage.

 

There is still a lot of guessing about what is THandlerClass, OnHandle and how AddHandler is defined and what it does with its parameter, so it is difficult to give a detailed analysis.

 

[Uwe Raabe 2057]

Now I have seen the link to the sample code. Will look into it.

[Dalija Prasnikar 1396]

31 minutes ago, Uwe Raabe said:

I imagine what is happening and I assume it to be expected behavior, although I admit one has to know how variable capturing works to get it. It is even documented (Variable Binding Mechanism)

 

There is still a lot of guessing about what is THandlerClass, OnHandle and how AddHandler is defined and what it does with its parameter, so it is difficult to give a detailed analysis.

 

You are on right track.

 

What happens is something like this:

 

procedure TTest.ConfigLoop;
var
  Handler: THandlerClass;
  hType: THandlers;
  hRef: TOnHandle;
begin
  Writeln;
  Writeln('--- ConfigLoop');
  Broker.Clear;

  for hType in [foo, bar]
  do begin
    case hType of
      foo: Handler := TFoo.Create;
      bar: Handler := TBar.Create;
      // wtf: ;
    end;
    hRef := Handler.OnHandle;
    // Capture does no discern between the foo instance and the bar instance
    Broker.AddHandler(hRef);
  end;
end;

 

and then this bridging assignment from plain method to anonymous method is expanded to

 

hRef := 
  procedure
  begin
    Handler.OnHandle();
  end;

Where before mentioned rules about capturing references (variables) and not values apply.

 

[M.Joos 30]

This is not a bug, but expected behavior: This is what variable capture with anonymous methods is all about.

But I agree, this can sometimes be hard to spot and lead to unexpected results as in your case.

[Attila Kovacs 629]

Mhm,

Quote

It is even documented (Variable Binding Mechanism)

 

Note that variable capture captures variables--not values

So avoid variables and everything should be ok in anon procs?

Like that?

 

    case hType of
      foo:
        //Handler := TFoo.Create;
        Broker.AddHandler(TFoo.Create.OnHandle);
      bar:
        //Handler := TBar.Create;
        Broker.AddHandler(TBar.Create.OnHandle);
    end;

 

Edited February 9, 2019 by Attila Kovacs

[Uwe Raabe 2057]

1 minute ago, Attila Kovacs said:

So avoid variables and everything should be ok in anon methods?

Lars also showed a valid approach in his blog post. There is nothing wrong with using variables. As always you have to know what you do.

[M.Joos 30]

7 minutes ago, Attila Kovacs said:

Mhm,

So avoid variables and everything should be ok in anon methods?

Like that?

 

    case hType of
      foo:
        //Handler := TFoo.Create;
        Broker.AddHandler(TFoo.Create.OnHandle);
      bar:
        //Handler := TBar.Create;
        Broker.AddHandler(TBar.Create.OnHandle);
    end;

 

Although I wouldn't phrase it like this, yes, this version should work. Variable capture is actually an important feature of anonymous methods. Without, they would hardly be more than a fancy way of procedural types.

[Attila Kovacs 629]

I don't want to hijack this thread but this is such a cool example, there is something I can't understand.

I have completed it with the corresponding destructors and free's, and it still leaks.

Is this an expected leak? Are TFoo and TBar instances stucked?

 

Edit: Okay, this could be also a deficit of the SCCE. Maybe he is managing them in his actual code. Anyway, the trick using an abstract class to passing its method as an anon proc is really great.

Edited February 9, 2019 by Attila Kovacs

[Lars Fosdal 1792]

Yes, the examples leak. I didn't want to clutter it up with too much housekeeping code. 

 

Thank you, Uwe, for enlightening me on variable vs value capture. IMO, the compiler could need a hint or warning if a variable capture happens in a loop, because it is really easy to overlook. 

 

I use a number of variations on this to do dependency injection. It really helps with avoiding pulling too much project specific code into general code and keep the libraries isolated from each other. 

 

This code in particular is part of a web server that (now correctly) supports a configurable collection of JsonRPC protocol handlers. The web server knows nothing about Json, and the protocol handlers knows almost nothing about http.

[Lars Fosdal 1792]

16 hours ago, David Heffernan said:

If we have no knowledge of the types involved in this code then for sure we can't know what's wrong with it. 

Which is why my post specifically pointed out that there is a link to the SCCE in the blog post?

[David Heffernan 2345]

41 minutes ago, Lars Fosdal said:

Which is why my post specifically pointed out that there is a link to the SCCE in the blog post?

Whatever. The text in the blog post didn't make the types clear. I wasn't about to download some project from a file sharing site. It's quite easy to make a complete program in a simple console app. There must be a few posts on this topic out there already. 

[Lars Fosdal 1792]

@David Heffernan If you added me to your Ignore list here on DP, I would be totally fine with that.

[David Heffernan 2345]

2 hours ago, Lars Fosdal said:

@David Heffernan If you added me to your Ignore list here on DP, I would be totally fine with that.

I'd still see all the replies. It's an interesting topic that you raised but it needs a clearer example to bring out the issue. 

[Lars Fosdal 1792]

15 hours ago, David Heffernan said:

I'd still see all the replies. It's an interesting topic that you raised but it needs a clearer example to bring out the issue. 

There is one, but you "wasn't about to download some project from a file sharing site" (i.e. a zip file with source code from Google Drive)

[David Heffernan 2345]

4 hours ago, Lars Fosdal said:

There is one, but you "wasn't about to download some project from a file sharing site" (i.e. a zip file with source code from Google Drive)

It's an extra step for every one of your readers. In fact it's multiple steps. Down file. Unzip file. One goal of the writer is to make it easier for the reader to see and understand. 

 

For instance, if I was on a mobile device how would I read files in a zip file? 

Edited February 10, 2019 by David Heffernan

[Dalija Prasnikar 1396]

On 2/9/2019 at 7:48 AM, Lars Fosdal said:

Which is why my post specifically pointed out that there is a link to the SCCE in the blog post?

To be honest, knowing the type of Handler.OnHandle is crucial for understanding the issue. The other implementation details don't matter as much. 

Broker.AddHandler(Handler.OnHandle);

And usually I don't go around and download code examples, just to get to the bottom of some problem. Only under special circumstances - mostly from bugs reported in QP, when having complete test project is crucial for reproducing the issue.

The only reason, why I did go and download this example is because I know you. 

So whether you like it or not, David does have a point.

[Rudy Velthuis 91]

On 2/8/2019 at 2:19 PM, Lars Fosdal said:

Code that looks correct, and appear to compile alright, but which doesn't execute well.
Can you spot the error? See my blog post for a link to a SCCE.

var
  Handler: THandlerClass;
  hType: THandlers;
begin
  Broker.Clear;
  for hType in [foo, bar]
  do begin
    case hType of
      foo: Handler := TFoo.Create;
      bar: Handler := TBar.Create;
    end;
    Broker.AddHandler(Handler.OnHandle);
  end;
end;

https://larsfosdal.blog/2019/02/08/delphi-pitfalls-of-anonymous-methods-and-capture/

Ok, sometimes (yeah, right, funny) I am dense, but what is SCCE? Source Code Compilable Example?

[Lars Fosdal 1792]

6 hours ago, Rudy Velthuis said:

Ok, sometimes (yeah, right, funny) I am dense, but what is SCCE? Source Code Compilable Example?

Close, its a Self-Contained Compilable Example, sometimes also called a SSCCE, i.e. a Short SCCE. 

 

I'll rewrite the article for better readability. 

 

[Dalija Prasnikar 1396]

3 hours ago, Lars Fosdal said:

Close, its a Self-Contained Compilable Example, sometimes also called a SSCCE, i.e. a Short SCCE. 

 

or MCVE Minimal, Complete, and Verifiable Example

[Attila Kovacs 629]

On the other side, I'm really happy that I had to download the sources to see what he has, and so I have seen a really great trick and the thread became not just a 42424th variable/value capturing rant.

 

However one thing is still not clear for me Lars, if your actual code takes care of freeing the TFoo and TBar instances, how comes that you instantiate them into a local variable in a loop?

[David Heffernan 2345]

11 hours ago, Rudy Velthuis said:

Ok, sometimes (yeah, right, funny) I am dense, but what is SCCE? Source Code Compilable Example?

It's a typo. Should be SSCCE: http://sscce.org/

[Rudy Velthuis 91]

1 hour ago, David Heffernan said:

It's a typo. Should be SSCCE: http://sscce.org/

Ah, thanks. I had seen something like it before, but when I googled, I found nothing useful. The extra S makes it easier to find too. <g>