C objects beyond their Lua lifetime, or: a new callback-registration invariant?

[nwf]

The Problem

We have a number of modules (notably, tmr, net, tls) that appear to be intended to function without the Lua application retaining reference to any particular object: it's OK to tmr.create():alarm(...) and similarly drop a socket object after calling :on() to establish callbacks.

This creates the unfortunate possibility that some application unregisters all callbacks and so leaves the underlying C object pinned in the registry, forever occupying resources and yet unreachable from the Lua object graph. It is difficult to detect this happening, since the hold from the registry prevents garbage collection, and so we cannot tell if the user is otherwise retaining reference or not.

New invariant proposal

Therefore, I propose that we introduce, document, and gradually enforce the following invariant:

Any callback-driven API object must have, at all times, least one callback for a possible eventuality from its current state registered before being told to initiate events; such an object is permitted to abort at any point in the future if this is not upheld.

This licenses the following kinds of behavior in lieu of resource leaks:

• a tmr object may stop itself, and release itself from the Lua registry, if, when a timer tick arrives, there is no callback registered. If the user has retained a reference, they may register a new callback and restart the timer via that reference (and it will re-register itself with the registry).

• a tmr object may interpret start as a no-op if there is no callback registered, and so may skip installing itself into the Lua registry.

• a tls or net connection may tear itself down from the established state if it has no receive, sent, or disconnect callback. If the user has retained a reference, they are free to reuse the connection (and it will re-register itself with the registry).

• a tls or net connection object is permitted to disconnect if, upon connection establishment, it finds no connected, receive, sent, or disconnected callback.

• a tls or net connection object is permitted to fail a connect request if there are no callbacks registered. (and so the user must write skt:on("connect", ...) skt:connect(...), not the other way around).

New requirement proposal

Alternatively, we could require that the state-machine objects (tmr objects, tls connections, ...) be held by the user and not pin them in the registry. This is attractive for its simplicity but has, I think, an unpleasant side-effect. Because Lua does not easily offer two-stage finalization (that is, while __gc metamethods can resurrect objects, there is no indication that this has taken place), these objects' __gc metamethods would unconditionally unregister the object's Lua callbacks, making resurrection challenging from an application perspective.

ETA: The reason to consider resurrection is that it would be most friendly if our __gc metamethods on, for example, sockets, attempted to call back on the "disconnected" callback, if there were one registered. If we aren't worried about being that friendly in the event of breaking this "must-be-held" invariant, then I think we don't need to worry about resurrection and it becomes the simplest proposal but with far-reaching code-breaking potential.

Call for help

Any and all feedback, including alternate proposals for dealing with these edge cases, is more than welcome.

[TerryE]

Nathaniel, I have a different perspective. The use of the Lua registry as an adjunct to the C API and its use on libraries is well thought out and self-consistent. If libraries are implemented correctly then Lua GC works fine and there isn't leakage. The challenge that we've got is that our implementations aren't always well thought out and so it is possible to code applications in such a way that they include circular references which include a Lua registry entry and thus frustrate Lua GC. This is an implementation error and not a Lua architectural error.

The suggestion that application libraries reimplement the registry functions a non-trivial piece of work that isn't going to remove such underlying implementation errors and which greatly complicate tracking them down. What we really need to do is to fix the logic flaw in our libraries.

CBs should be registered if and only if they are validly scoped, and more to the point CBs should only refer to their parent object as a CB parameter and not as an upvalue. So taking tmr as an example.

• One-shots should always unregister the CB.
• We should have a way of cancelling an existing timer and such library functions should disarm any corresponding OS timers.
• The documentation should emphasise that a timer CB has the timer object as a parameter. whereas all tmr CB examples don't mention the CB object wrong

Here is a little example that demonstrates that Lua can do this descoping and GC properly. Running this do block cleans up after itself and leaves nothing in _G or the registry and no heap growth.

do 
  local i, t, a, b = 0, tmr.create()
  local function pr()
    local r = debug.getregistry()
    for i = 0,#r+1 do print (i,r[i]) end
  end
  function a(t) uart.write(0, "."); t:alarm(200,0,b) end
  function b(t) uart.write(0, "-") 
    i = i+1
    if i > 20 then pr() else t:alarm(500,0,a) end
  end
  pr(); a(t)
end

• When CBs are no longer required we should eagerly (rather than lazily) clean them up. So if the object has reached a state where the CB can't be called then it should be removed. A good exmple of where this is flaky is in the net object where you sometimes have to jump through hoops to GC the CB entries. No. If you close a listener then the code should clean up CBs properly.

[nwf]

Thanks for chiming in, @TerryE. Your input is much appreciated, though I think you have, perhaps, taken the wrong message from my comment. I must have been imprecise; let me try again.

I don't doubt that Lua can GC correctly; if that wasn't clear in my original message, I apologize. In no way was I meaning to suggest that modules re-implement registry functionality, merely that we have to handle the registry differently than we presently do.

I appreciate the desire to un-hook callbacks as soon as possible, but consider, for example, a tls connection object: it may be used repeatedly to make different connections, and so its callbacks are always required -- even if the current connection is well past calling the dns callback, it can't be de-registered because it might be called on the next connection. So yes, but that doesn't, and can't, change our tls module as presently designed.

Precisely because of the guidance that our callbacks should only ever refer to their (e.g.) tls objects through callback arguments, our API suggests that it is entirely fine to drop all references on the Lua side and let the fact that there's a C-side connection object holding the object (and its callbacks) in the registry drive everything. But this creates the opportunity for a bug; if I go nil out all of the callbacks on an active tls connection, it will remain pinned with no way for Lua to do anything about it.

I want to amend our APIs to say that if an event arrives with no callbacks registered (not just for the event in question, but for any possible successor state) then we are allowed to drop the connection, even if the object is otherwise referenced from Lua, because we cannot readily distinguish between the connection merely being pinned in the registry for LwIP's sake or because it's also retained by the user.

A worked implementation of this (and of correcting use-after-free bugs by explicit refcounting) for tls is available in my dev tree; see https://github.com/nwf/nodemcu-firmware/blob/dev-active/app/modules/tls.c . (Forgive me if I don't link to an actual commit, since my tree tends to see a lot of rebasing and push -fing.)

[TerryE]

@nwf, please don't apologise for me being out of the loop. I am in the process of catch-up and re-engaging. I will PM you separately tomorrow.

But this creates the opportunity for a bug; if I go nil out all of the callbacks on an active tls connection, it will remain pinned with no way for Lua to do anything about it.

A function variable is a reference to an active Lua closure, and just like any other reference you can nil or use other mechanisms to let the variable go out of scope. The closure itself remains so long as a GC sweep can mark it. In this case there is also a valid reference in the Lua registry to the closure so it is still active and still referenced, albeit just through the registry. This is just the same as doing a tmr.create():alarm(n,0,someCB) which leaves 2 references in the registry, one for the tmr object and one for the CB even though there are no inscope variables outside of the registry referencing these.

Perhaps we can chat 1-1 about some of this tomorrow. //T

[nwf]

Sorry, I was again imprecise; by "nil out the callbacks", I meant call skt:on("foo", nil) to cause the C module to drop the registry entries for the callback(s).

A 1:1 sounds good to me. If Hangouts is agreeable, ....@gmail.com will do just fine. :)
Cheers!

[nwf]

Just to chime in with another thought: one source of knots tied through the registry is that we sometimes see the from-C rx-side event callbacks hold on to objects (via upvals) that (transitively) hold on to the C object. One way to break this would be for the constructor to register its UD with the registry and return a proxy object. We can augment the constructor to allow the user to indicate whether the proxy getting GC'd is a reason to tear down the registry entry or whether the object should persist and continue to fire callbacks.

[TerryE]

We need to be a little careful on __GC handling because sometimes we want object (hierarchies) to be collected on dereferencing and sometimes (e.g. tmr.create():alarm(2000,0,doCleanup)) where we don't want clean-up to occur until the CB method has fired). Whatever some modules like net have bad behaviours (e.g. closing a socket does not clear down socket resources).

I think we should develop these guideline after reviewing the worst offenders like net

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[nwf]

Still an issue