feross / feross/simple-peer

Race condition in destroy method

Open
#848 0 comments 3 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
7.8k
Forks
983
PR merge metrics
No merged PRs in 30d

Description

**What version of this package are you using?**
Version: Commit d1c0ebe
Platform: Windows 10, Chrome Version 94.0.4606.81 (Official Build) (64-bit)

**Problem:**
peer.destroy() method has potential race condition

**How (my interpretation)**
It unhooks `peer._channel.onclose`, to avoid infinite recursion (since closing data channel will invoke `peer.destroy()` method)
Then calls `_channel.close()` and `_pc.close()` sequentially.
But order of execution is not guaranteed to be the same as they were called

Occasionally peer connection is closed before data channel is closed, causing remote peer to throw "RTCError Transport channel closed".

**Expected:**
When peer.destroy() method is called, remote peer should not experience any error.

**Work around:**
call `peer._channel.close()` to destroy peer

**Writing pull request:**
I am reluctant to write a pull request since I do not have full picture of the library and I may introduce more bugs with my own changes.
I will only write pull request if I am sure it is bug free.
My intent is to submit this issue for bug confirmation.

**Reproducible code:**
```
// Run it few more times, error should occur randomly
peerA = new Peer({ initiator: true });
peerB = new Peer({ initiator: false });
peerA.on('signal', s => peerB.signal(s));
peerB.on('signal', s => peerA.signal(s));
peerA.on("error", console.warn);
peerB.on("error", console.warn);
setTimeout(peerA.destroy.bind(peerA), 100);

// Work around
// setTimeout(peerA._channel.close.bind(peerA._channel), 100)
```

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.