@roamhq/wrtc (node-webrtc) emits/keeps late events after pc.close() causing Node test file to fail at 1:1 even though all subtests pass

[collapsinghierarchy]

I’m a bit stuck. I’ve tried just about everything, and while all my non-WebRTC tests pass, any test that wires in WebRTC fails due to something that seems to happen outside the actual test. This might be my unfamiliarity with Node’s test runner, but I haven’t been able to pin it down yet.

Environment

• OS: Windows 11 + WSL2 Ubuntu (fill in exact distro/kernel)
• Node.js: v20.19.4
• @roamhq/wrtc@0.9.0
• Repro is pure datachannel (no media), local host ICE only

Using a tiny in-process WebSocket broker and two wrtc peers (A initiator, B responder) (see the attached and self-contained test file):

• Both non-trickle and trickle open a DataChannel successfully
• Messages ping/echo both ways
• We then close the peers and the broker and wait a tick
• All subtests pass, but Node’s test runner still reports the file failed at line 1:1 (“anonymous test”), i.e., something fires after tests complete.

We’ve already:

• Made teardown idempotent (clear timers, remove every listener, null pc.on..., close DC if not closed)
• Silenced ws socket/server 'error' events
• Made the broker’s close() idempotent and terminate any clients
• Added a global process.on('uncaughtException'/'unhandledRejection') trap so late throws are captured (final test confirms no late errors)
• Ensured exactly-one EoC (candidate:null) in trickle mode (send on DC open if not seen)
  Still, the file fails at 1:1 even though the last two tests pass.

Expected

After we close both peer connections and the broker, the process should go idle and the test file should finish without a top-level failure.

Actual

`=== CASE trickle=false ===
[A] setLocal(offer)
gather change -> complete
[A] sent offer (non-trickle, completed=true, waitMs=53)
...
✔ wrtc e2e (non-trickle...) (…ms)

=== CASE trickle=true ===
… (similar, with EoC on open)
✔ wrtc e2e (trickle...) (…ms)
✔ no late errors after wrtc e2e (…ms)
ℹ tests 4
ℹ suites 0
ℹ pass 3
ℹ fail 1
✖ /path/to/wrtc_e2e.test.mjs (2719.757474ms)
'test failed'`

"Minimal" Repro

Dependencies: npm i @roamhq/wrtc ws

wrtc_e2e.test.js

Notes

• The failure seems not to be an unhandled exception. it looks either
  • Some internal handle stays open post-teardown (libwebrtc thread/socket) and the test runner flags the file, or
  • A late event on a closed PC/DC fires outside the tests and triggers Node’s runner behavior.
• If there’s a recommended canonical teardown order for wrtc to fully quiesce (beyond pc.close() + removing listeners), please advise. If any thread needs a tick to join, we can await a promise/hook.

Thanks a ton!

[duvallj]

Turns out this isn't just a duplicate of #35 , huh! Here's the backtrace I get when running a debug build:

* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x0000000198cb6388 libsystem_kernel.dylib`__pthread_kill + 8
    frame #1: 0x0000000198cef88c libsystem_pthread.dylib`pthread_kill + 296
    frame #2: 0x0000000198bf8a3c libsystem_c.dylib`abort + 124
    frame #3: 0x000000011cad0dc0 wrtc.node`std::__terminate(func=(libsystem_c.dylib`abort)) at cxa_handlers.cpp:59:9
    frame #4: 0x000000011cacf74c wrtc.node`__cxxabiv1::failed_throw(exception_header=0x000060000363d200) at cxa_exception.cpp:152:5
    frame #5: 0x000000011cacf690 wrtc.node`__cxa_throw(thrown_object=0x000060000363d280, tinfo=0x000000011e96b920, dest=(wrtc.node`Napi::Error::~Error() at napi.h:1787)) at cxa_exception.cpp:283:5
    frame #6: 0x000000011cbbc178 wrtc.node`node_webrtc::AsyncObjectWrap<node_webrtc::RTCPeerConnection>::MakeCallback(this=0x000000014d01d200, name=<unavailable>, args=0x000000016fdef578) at async_object_wrap.hh:0 [opt]
    frame #7: 0x000000011cbc1758 wrtc.node`node_webrtc::Callback<node_webrtc::RTCPeerConnection::OnDataChannel(rtc::scoped_refptr<webrtc::DataChannelInterface>)::$_9, node_webrtc::RTCPeerConnection>::Dispatch(node_webrtc::RTCPeerConnection&) [inlined] node_webrtc::RTCPeerConnection::OnDataChannel(rtc::scoped_refptr<webrtc::DataChannelInterface>)::$_9::operator()(this=<unavailable>) const at rtc_peer_connection.cc:216:5 [opt]
    frame #8: 0x000000011cbc16c8 wrtc.node`node_webrtc::Callback<node_webrtc::RTCPeerConnection::OnDataChannel(rtc::scoped_refptr<webrtc::DataChannelInterface>)::$_9, node_webrtc::RTCPeerConnection>::Dispatch(this=<unavailable>, (null)=<unavailable>) at events.hh:43:33 [opt]
    frame #9: 0x000000011cbb96f0 wrtc.node`node_webrtc::EventLoop<node_webrtc::RTCPeerConnection>::Run(this=0x000000014d01d200) at event_loop.hh:60:16 [opt]
    frame #10: 0x0000000102d0e224 libuv.1.dylib`uv__async_io + 276
    frame #11: 0x0000000102d22a28 libuv.1.dylib`uv__io_poll + 1036
    frame #12: 0x0000000102d0e92c libuv.1.dylib`uv_run + 424
    frame #13: 0x00000001000375c0 node`node::SpinEventLoopInternal(node::Environment*) + 272
    frame #14: 0x000000010015e560 node`node::NodeMainInstance::Run() + 276
    frame #15: 0x00000001000d8468 node`node::Start(int, char**) + 804
    frame #16: 0x000000019894eb98 dyld`start + 6076

Interpreting this, the actual code that's erroring out is this (the this->Value() line):

node-webrtc/src/node/async_object_wrap.hh

Lines 37 to 51 in 0a282dc

	protected:
	void MakeCallback(const char *name,
	const std::initializer_list<napi_value> &args) {
	auto self = this->Value();
	auto maybeFunction = self.Get(name);
	if (maybeFunction.IsFunction()) {
	_async_context_mutex.lock();
	if (_async_context) {
	napi_async_context context = *_async_context;
	maybeFunction.template As<Napi::Function>().MakeCallback(self, args,
	context);
	}
	_async_context_mutex.unlock();
	}
	}

As called from here:

node-webrtc/src/interfaces/rtc_peer_connection.cc

Lines 214 to 222 in 0a282dc

	void RTCPeerConnection::OnDataChannel(
	rtc::scoped_refptr<webrtc::DataChannelInterface> channel) {
	auto observer = new DataChannelObserver(_factory, channel);
	Dispatch(CreateCallback<RTCPeerConnection>([this, observer]() {
	auto channel =
	_data_channel_wrap.GetOrCreate(observer, observer->channel());
	MakeCallback("ondatachannel", {channel->Value()});
	}));
	}

So I think your guess is correct: it seems there's an ondatachannel event being sent to a destroyed RTCPeerConnection object.

I am not exactly sure how this is happening; I had thought that the Ref() and Unref() calls in the Napi::Reference implementation (from Napi::ObjectWrap, from our own node_webrtc::AsyncObjectWrap and node_webrtc::AsyncObjectWrapWithLoop classes) would be compatible enough to ensure the lifetime of the RTCPeerConnection was stretched long enough for these events to always be valid, but I guess not? If that's the case, then this is probably the buggy code:

node-webrtc/src/interfaces/rtc_peer_connection.cc

Lines 97 to 108 in 0a282dc

	auto deps = webrtc::PeerConnectionDependencies(this);
	deps.allocator = std::move(portAllocator);
	auto maybePeerConnection = _factory->factory()->CreatePeerConnectionOrError(
	configuration.configuration, std::move(deps));
	if (!maybePeerConnection.ok()) {
	Napi::Error::New(env, maybePeerConnection.MoveError().message())
	.ThrowAsJavaScriptException();
	return;
	}
	_jinglePeerConnection = maybePeerConnection.MoveValue();

The doc comment on PeerConnectionDependencies says "All new dependencies should be added as a unique_ptr to allow the PeerConnection object to be the definitive owner of the dependencies lifetime making injection safer." So I guess because the PeerConnectionObserver doesn't have any Ref()/Unref() methods defined on it, we actually aren't bound to the lifetime of the _jinglePeerConnection object; it can live longer than our containing class, which is bad.

Not sure how to start fixing this (similar to #35 unfortunately). Requires a big rethink of how I want to make libwebrtc & napi lifetimes play nice together.

[duvallj]

Anyways, thanks for the report, sorry I don't have much guidance on what else to do. This exception is being thrown from C++ code so I am not sure how you'd handle that from the node side. In general this library has trouble w/ teardown, might have to ignore those sorts of errors somehow in the meantime until this gets fixed.

[collapsinghierarchy]

Anyways, thanks for the report, sorry I don't have much guidance on what else to do. This exception is being thrown from C++ code so I am not sure how you'd handle that from the node side. In general this library has trouble w/ teardown, might have to ignore those sorts of errors somehow in the meantime until this gets fixed.

No problem. It's good enough to have a hypothesis validated. Thank you for your work! Maybe i'll be able to find the time to look into the c++ code and try to reproduce the backtrace you are seeing.