fix(#3): callback-safe Lua API — guards and API failures return, never Go-RaiseError - #8
Merged
Merged
Conversation
golua RaiseError is a Go panic that bypasses the LuaJIT VM unwinder. When it fires inside a callback and is recovered (callLuaCallback continues on the same lua_State), the VM's cframe/base/top are left dangling and the process crashes later (SIGBUS/SEGV in the main loop). The merged raiseIfInCallback guard is itself this corrupting pattern. char.read/write already report failures as (nil, msg). Make the in-callback guard use the same channel: return (nil, "... not allowed inside a subscribe/PTY callback ...") instead of RaiseError. No Go panic, no corruption; the script handles it like any read/write error. The write guard moves to the top of the method so it fires regardless of argument shape. First commit of the #3 re-fix; blim.sleep/io.read (Lua-side raise) and the subscribe guard removal follow.
io.read releases stateMutex to block, so an in-callback call is a true reentrancy-corruption vector and must be stopped before the release. But the stop must not be a Go-side RaiseError (a Go panic that bypasses the LuaJIT VM unwinder and corrupts the recovered lua_State). io.read already returns nil on EOF, so the guard fails the same way: return (nil, "... not allowed inside a subscribe/PTY callback ...") before touching the stdin machinery / releasing the mutex. Scripts handle it like EOF. Second commit of the #3 re-fix; blim.sleep (Lua-side raise) and the subscribe guard removal follow.
blim.sleep releases stateMutex, so an in-callback call is a true reentrancy-corruption vector. The stop must happen before the release AND must not be a Go-side RaiseError (a Go panic that bypasses the LuaJIT VM unwinder and corrupts the recovered lua_State). Return (nil, "... not allowed inside a subscribe/PTY callback ...") before releasing the mutex instead. The #4 cancellation raise (ctx.Done -> RaiseError) is unchanged: it is terminal (the script aborts and the state is discarded), so the Go panic is safe there, and it must stay a Go raise to remain uncatchable by Lua pcall. Third commit of the #3 re-fix; removing the subscribe guard (and the now-unused raiseIfInCallback) follows.
blim.subscribe was the trigger of the reopened crash: its in-callback guard raised a Go-side error, and that recovered Go panic is what corrupts the lua_State on LuaJIT. subscribe never releases stateMutex and never re-enters Lua — its CCCD write completes via the go-ble connection-event path, independent of the notification fan-out — so from a callback it is at most a brief stall, not corruption. Remove the guard entirely (making it non-blocking is a separate follow-up). With the read/write/io.read/sleep guards now failing by return, raiseIfInCallback has no callers left; delete it. inCallback() remains (guards + shutdown hook).
The original guard tests were green because they did almost no VM work after the in-callback raise, so the corruption never surfaced. Add a test that, after a callback invokes a guarded op (blim.sleep -> returns nil,err), runs 100k protected calls in the main loop and asserts the engine still works. On the old Go-side RaiseError guard this corrupted the recovered lua_State and SIGBUS'd under exactly this kind of later work; with the return-based guards it is safe by construction.
…uards Comment-only: the group doc still described the old "reject with a clean, recoverable Lua error" behavior and listed blim.subscribe as guarded. Update it to reflect that guards now fail by returning (nil, msg) and that subscribe is no longer guarded.
A characteristic lookup miss is an expected, queryable RUNTIME condition, not an error — the original repro even relies on it: `local char = blim.characteristic( ..); if char then char.read() end`, which is dead code if the lookup raises. blim.characteristic now RETURNS nil on not-found and no-connection (universal, main loop and callback), matching that contract. This also closes the likely original crash path: from a callback the old Go-side RaiseError (a Go panic) corrupted the recovered lua_State and crashed the process later. Returning nil avoids it. Argument validation (wrong types) stays a raise — that is genuine misuse and should fail fast. Updates the main-loop not-found tests (were asserting the buggy raise) to assert nil, and adds a callback regression test: a missing lookup in a callback returns nil and the engine survives heavy subsequent VM work.
subscribe is allowed from a callback (dynamic subscription on a state change),
so its failure path must not Go-raise: a recovered Go panic corrupts the
lua_State on LuaJIT. Config / runtime failures (no services, missing service or
characteristic, no callback) now RETURN (nil, err); scripts check it via
`local cancel, err = blim.subscribe{...}`. A wrong argument TYPE (not a table)
stays a raise — that is misuse and should fail fast.
Tests now assert the return contract directly (were asserting the buggy raise),
and the scenario template re-raises the returned error via a Lua-side error() so
the ScriptError-based error scenarios still work. Adds a callback regression
test: a failing subscribe in a callback returns (nil, err) and the engine
survives heavy subsequent VM work.
The guard sat after the argument-validation raises, so blim.sleep("badarg")
from a callback hit a Go-side RaiseError (which corrupts the recovered
lua_State) before the guard could return. Move the in-callback guard to the top
of the function so any sleep from a callback returns (nil, msg) regardless of
argument shape; the argument-validation raise is now reachable only from the
main loop, where it is safe.
…ck test
The "blim.subscribe inside a callback is allowed" test created a second
subscription (180d/2a37) from the callback and never cancelled it, so it lingered
until connection teardown. On a busy/slow CI run the subscription-cleanup
diagnostic (a per-connection value-pool check that runs during CancelAll) then
saw that still-active subscription's outstanding values while another
subscription was being cancelled and panicked ("blevalue_pool: outstanding
increased") — the same multi-subscription unreliability the diagnostic already
dropped for its goroutine-delta check.
Capture the nested cancel handle and cancel it from the main context after the
assertion so only one subscription is live at teardown. This is a test-hygiene
fix: subscribe-from-callback is registered via the same SubscribeWithName ->
manager.Add path and cancelled by Disconnect -> CancelAll like any subscription,
so there is no production leak.
…iption
The per-subscription blevalue_pool check in verifyExplicitCancel compares the
connection's outstanding-value count captured at this subscription's cancel with
the count at its fan-out-goroutine exit. The value pool is shared per-connection,
so when 2+ subscriptions are active the window picks up values that OTHER
subscriptions' in-flight notifications allocated during teardown — a false
positive that hard-panics ("blevalue_pool: outstanding increased"). It is
timing-dependent: it flakes under CI load and passes locally, and it hit both
this branch and main (e.g. run 93b00f9, sub-365 0 -> 8).
This is the same multi-subscription unreliability that already retired the
goroutine-delta check right above it. Gate the pool check on
ConnDiag.SubscriptionCount() == 1 so it only runs when this is the sole
subscription on the connection (no concurrent allocators). Multi-subscription
leaks are still caught by VerifyDisconnectCleanup's blevalue_pool_disconnect
check, which runs after CancelAll + Wait when nothing else can allocate. The gate
is entirely in test-build diagnostics; no production code changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Re-fixes #3. The previously-merged in-callback guard was itself a corruption source: golua's
RaiseErroris a Go panic, not alua_errorlongjmp. When it fires inside a callback and is recovered (execution continues on the samelua_State), the LuaJIT VM'scframe/base/topare left dangling — the VM unwinder never runs — and the process crashes later (SIGBUS/SEGV in the main loop). The original green tests missed it because they did almost no VM work after the raise.Fix: everything that is unsafe or fallible inside a callback fails by returning a value, never via a Go-side raise. Only genuine misuse (wrong argument types) still raises.
Changes (one concern per commit)
(nil, msg)inside a callback (guard at the top, before arg validation).(nil, msg)inside a callback.(nil, msg)inside a callback; guard moved before argument validation so a bad-arg sleep from a callback also returns instead of raising. The Ctrl+C hangs forever: cancellation count-hook is not honored in LuaJIT-compiled traces; blim.sleep should raise on ctx.Done #4 cancellation raise (ctx.Done) stays Go-side — it is terminal and must be uncatchable bypcall.(nil, err)instead of raising, so a failing subscribe from a callback cannot corrupt. Not-a-table (misuse) still raises.if char then ...). This closes the likely original crash path. Arg validation (wrong types) still raises.inCallback()remains.Tests
(nil, msg)from a callback, does not raise).blim.characteristicnot-found from a callback, and a failingblim.subscribefrom a callback.error().Whole module
-race -count=1clean;go vet+gofmtclean.Follow-ups (separate issues, out of scope)
blim.subscribe(remove the synchronous CCCD-write stall).blim.defer(fn)— sanctioned continuation to run a guarded op from a callback in the main context (Go-driven, no active-loop requirement).Fixes #3