Jump to content
Sign in to follow this  
Lars Fosdal

Pitfalls of Anonymous methods and capture

Recommended Posts

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/

Share this post


Link to post

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.

Share this post


Link to post

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.

 

  • Like 1
  • Thanks 1

Share this post


Link to post
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.

 

  • Like 1

Share this post


Link to post

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.

  • Thanks 1

Share this post


Link to post

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 by Attila Kovacs

Share this post


Link to post
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.

  • Like 1

Share this post


Link to post
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.

Share this post


Link to post

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 by Attila Kovacs
  • Thanks 1

Share this post


Link to post

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.

  • Like 1

Share this post


Link to post
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?

Share this post


Link to post
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. 

Share this post


Link to post
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. 

Share this post


Link to post
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)

Share this post


Link to post
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 by David Heffernan
  • Like 3

Share this post


Link to post
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.

  • Like 4

Share this post


Link to post
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?

Share this post


Link to post
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. 

 

Share this post


Link to post
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

  • Like 1

Share this post


Link to post

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?

Share this post


Link to post
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>

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
Sign in to follow this  

×