Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closures are not properly maintained across event handlers. #734

Open
ZekeMedley opened this issue Jan 10, 2020 · 3 comments
Open

Closures are not properly maintained across event handlers. #734

ZekeMedley opened this issue Jan 10, 2020 · 3 comments

Comments

@ZekeMedley
Copy link
Member

@ZekeMedley ZekeMedley commented Jan 10, 2020

As @jsiwek mentions here, a closure that is made in an event does not properly persist across different handlers for that event. For example:

global my_func: function();

event zeek_init() &priority=+10
	{
	local outer = 100;

	local lambda = function()
		{
		print outer;
		};

	lambda(); # prints 100
	my_func = lambda;
	my_func(); # prints 100
	}

event zeek_init() &priority=-10
	{
	local qux = 13;
	my_func(); # prints 13, but expected 100 ?
	}
$ zeek -b test.zeek 
100
100
13

In the comment linked above, Jon writes:

That output wasn't what I expected: when the lambda function escapes the scope in which it was created, I'd expect it to freeze the value of anything it was referencing in the enclosing Frame. But here, we see the result the Frame being shared across event handler bodies, with outer and qux sharing the same Frame offset and so the lookup of outer from within the escaped-lambda just evaluates to that value assigned to qux since they're actually just sharing the same Frame location.

@ZekeMedley

This comment has been minimized.

Copy link
Member Author

@ZekeMedley ZekeMedley commented Jan 16, 2020

@rsmmr and I just talked about this.

Robin mentioned that one option for handling this sort of edge case for the moment is to try and detect edge cases and surface a warning. For example, with this one we could show a warning or error if a Frame's Reset() method is triggered while it is a closure. This isn't ideal behavior, but it seems better than the current, broken behavior.

@jsiwek can you think of any other cases we might want to warn about? In particular, is there is a place you're worried about cycles causing leaks where we might be able to raise a warning?

@ZekeMedley

This comment has been minimized.

Copy link
Member Author

@ZekeMedley ZekeMedley commented Jan 16, 2020

W.R.T fixing this error, I recently read a nice paper called The Implementation of Lua 5.0. In section 5 they discuss how Lua handles closures and I think their method would work very nicely for Zeek.

Basically, their functions store non-owning pointers to variables in their closures until those variables would be deleted at which point they take ownership of them. It would require some architectural changes to make our closures behave like that, but most of the hard work is already done.

I may take that on next I get to spend time with Zeek.

@jsiwek

This comment has been minimized.

Copy link
Member

@jsiwek jsiwek commented Jan 16, 2020

Robin mentioned that one option for handling this sort of edge case for the moment is to try and detect edge cases and surface a warning. For example, with this one we could show a warning or error if a Frame's Reset() method is triggered while it is a closure. This isn't ideal behavior, but it seems better than the current, broken behavior.

If we find the right place/logic to issue a warning in precisely the scenario where the lambda has escaped the enclosing event handler Frame and that Frame is going away (getting reset in event handler case), then I'd guess it's not much further work to actually implement better behavior?

E.g. instead of emit a warning, my intuition is it could just copy the Frame at that point. It's true that multiple lambdas could have been mutating things in that shared Frame and if both escape, but could still just make a single copy that they all still share, or even think it could be fine/intuitive if we decide the semantics of that is just that every lambda then gets an independent copy to work on.

@jsiwek can you think of any other cases we might want to warn about? In particular, is there is a place you're worried about cycles causing leaks where we might be able to raise a warning?

Only other scenario that isn't well-tested, but that I wonder about at the moment would involve recursion or other ways a lambda might get a self-reference in a roundabout way.

Basically, their functions store non-owning pointers to variables in their closures until those variables would be deleted at which point they take ownership of them. It would require some architectural changes to make our closures behave like that, but most of the hard work is already done.

Didn't read the Lua paper, but sounds similar to what I already ended up doing: a Function initially marks the closure Frame as a weak reference, but the closure Frame holds a strong reference to the Function until that Frame is about to be destroyed. At that point, if the Frame finds that it's the only thing holding a reference to the Function, both Frame and Function can be deleted as normal, but if there's other outstanding references to the Function, it means that Function has escaped its enclosing Frame and so it then makes a copy of its closure Frame and marks it as a strong reference to take ownership.

So main difference is probably whether taking ownership of the closure Frame means making a copy or just converting to strong reference -- my logic took place in the Frame destructor, so copy was the only option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.