From a6fc9d304c42bd831a25838664063498fb1c4fcd Mon Sep 17 00:00:00 2001 From: Takashi Kannan <26959415+kannan-xiao4@users.noreply.github.com> Date: Mon, 14 Jun 2021 18:33:30 +0900 Subject: [PATCH] Fix: webserver error (#497) * check exsit pair before access on offer/answer * fix test * check client exist when delete connection * fix delete process * fix and add test about ReNegotiationAfterReceivingFirstOffer * wait while not stable on sendoffercorutine * controll offer answer order * wait to stable on manage --- WebApp/public/bidirectional/js/peer.js | 2 +- WebApp/public/bidirectional/js/sendvideo.js | 10 ++- WebApp/public/js/signaling.js | 9 +- WebApp/src/signaling.ts | 34 +++++--- WebApp/src/websocket.ts | 50 +++++------ .../Scripts/RenderStreamingInternal.cs | 14 ++- .../Scripts/Signaling/HttpSignaling.cs | 3 +- .../Tests/Runtime/PrivateSignalingTest.cs | 6 -- .../Runtime/RenderStreamingInternalTest.cs | 85 +++++++++++++++++-- .../Tests/Runtime/Signaling/MockSignaling.cs | 2 +- 10 files changed, 149 insertions(+), 66 deletions(-) diff --git a/WebApp/public/bidirectional/js/peer.js b/WebApp/public/bidirectional/js/peer.js index f53f262d3..19a7bad01 100644 --- a/WebApp/public/bidirectional/js/peer.js +++ b/WebApp/public/bidirectional/js/peer.js @@ -68,7 +68,7 @@ export default class Peer extends EventTarget { async loopResendOffer() { while (true) { - if(this.waitingAnswer) { + if(this.pc != null && this.waitingAnswer) { this.dispatchEvent(new CustomEvent('sendoffer', { detail: { connectionId: this.connectionId, sdp: this.pc.localDescription.sdp } })); } await this.sleep(this.interval); diff --git a/WebApp/public/bidirectional/js/sendvideo.js b/WebApp/public/bidirectional/js/sendvideo.js index 0bcfd7ad2..fc3f58443 100644 --- a/WebApp/public/bidirectional/js/sendvideo.js +++ b/WebApp/public/bidirectional/js/sendvideo.js @@ -39,7 +39,7 @@ export class SendVideo { this.signaling.addEventListener('disconnect', async (e) => { const data = e.detail; - if (_this.pc.connectionId == data.connectionId) { + if (_this.pc != null && _this.pc.connectionId == data.connectionId) { _this.ondisconnect(); } }); @@ -57,13 +57,17 @@ export class SendVideo { this.signaling.addEventListener('answer', async (e) => { const answer = e.detail; const desc = new RTCSessionDescription({ sdp: answer.sdp, type: "answer" }); - await _this.pc.onGotDescription(answer.connectionId, desc); + if (_this.pc != null) { + await _this.pc.onGotDescription(answer.connectionId, desc); + } }); this.signaling.addEventListener('candidate', async (e) => { const candidate = e.detail; const iceCandidate = new RTCIceCandidate({ candidate: candidate.candidate, sdpMid: candidate.sdpMid, sdpMLineIndex: candidate.sdpMLineIndex }); - await _this.pc.onGotCandidate(candidate.connectionId, iceCandidate); + if (_this.pc != null) { + await _this.pc.onGotCandidate(candidate.connectionId, iceCandidate); + } }); await this.signaling.start(); diff --git a/WebApp/public/js/signaling.js b/WebApp/public/js/signaling.js index 0292669c9..44f1414c3 100644 --- a/WebApp/public/js/signaling.js +++ b/WebApp/public/js/signaling.js @@ -5,6 +5,7 @@ export default class Signaling extends EventTarget { constructor() { super(); this.interval = 3000; + this.running = false; this.sleep = msec => new Promise(resolve => setTimeout(resolve, msec)); } @@ -25,6 +26,7 @@ export default class Signaling extends EventTarget { const createResponse = await fetch(this.url(''), { method: 'PUT', headers: this.headers() }); const session = await createResponse.json(); this.sessionId = session.sessionId; + this.running = true; this.loopGetOffer(); this.loopGetAnswer(); @@ -34,7 +36,7 @@ export default class Signaling extends EventTarget { async loopGetOffer() { let lastTimeRequest = Date.now() - 30000; - while (true) { + while (this.running) { const res = await this.getOffer(lastTimeRequest); lastTimeRequest = Date.parse(res.headers.get('Date')); @@ -54,7 +56,7 @@ export default class Signaling extends EventTarget { // receive answer message from 30secs ago let lastTimeRequest = Date.now() - 30000; - while (true) { + while (this.running) { const res = await this.getAnswer(lastTimeRequest); lastTimeRequest = Date.parse(res.headers.get('Date')); @@ -74,7 +76,7 @@ export default class Signaling extends EventTarget { // receive answer message from 30secs ago let lastTimeRequest = Date.now() - 30000; - while (true) { + while (this.running) { const res = await this.getCandidate(lastTimeRequest); lastTimeRequest = Date.parse(res.headers.get('Date')); @@ -93,6 +95,7 @@ export default class Signaling extends EventTarget { } async stop() { + this.running = false; await fetch(this.url(''), { method: 'DELETE', headers: this.headers() }); this.sessionId = null; } diff --git a/WebApp/src/signaling.ts b/WebApp/src/signaling.ts index a2cc42c0f..c0ae0cd9d 100644 --- a/WebApp/src/signaling.ts +++ b/WebApp/src/signaling.ts @@ -180,14 +180,16 @@ router.delete('/connection', (req: Request, res: Response) => { const pair = connectionPair.get(connectionId); const otherSessionId = pair[0] == sessionId ? pair[1] : pair[0]; if (otherSessionId) { - clients.get(otherSessionId).delete(connectionId); + if (clients.has(otherSessionId)) { + clients.get(otherSessionId).delete(connectionId); + } } } connectionPair.delete(connectionId); offers.get(sessionId).delete(connectionId); answers.get(sessionId).delete(connectionId); candidates.get(sessionId).delete(connectionId); - res.sendStatus(200); + res.json({ connectionId: connectionId }); }); router.post('/offer', (req: Request, res: Response) => { @@ -197,20 +199,21 @@ router.post('/offer', (req: Request, res: Response) => { let polite = false; if (res.app.get('isPrivate')) { - const pair = connectionPair.get(connectionId); - keySessionId = pair[0] == sessionId ? pair[1] : pair[0]; - if (keySessionId == null) { - const err = new Error(`${connectionId}: This connection id is not ready other session.`); - console.log(err); - res.status(400).send({ error: err }); - return; + if (connectionPair.has(connectionId)) { + const pair = connectionPair.get(connectionId); + keySessionId = pair[0] == sessionId ? pair[1] : pair[0]; + if (keySessionId != null) { + polite = true; + const map = offers.get(keySessionId); + map.set(connectionId, new Offer(req.body.sdp, Date.now(), polite)) + } } - polite = true; - } else { - connectionPair.set(connectionId, [sessionId, null]); - keySessionId = sessionId; + res.sendStatus(200); + return; } + connectionPair.set(connectionId, [sessionId, null]); + keySessionId = sessionId; const map = offers.get(keySessionId); map.set(connectionId, new Offer(req.body.sdp, Date.now(), polite)) @@ -223,6 +226,11 @@ router.post('/answer', (req: Request, res: Response) => { const connectionIds = getOrCreateConnectionIds(sessionId); connectionIds.add(connectionId); + if (!connectionPair.has(connectionId)) { + res.sendStatus(200); + return; + } + // add connectionPair const pair = connectionPair.get(connectionId); const otherSessionId = pair[0] == sessionId ? pair[1] : pair[0]; diff --git a/WebApp/src/websocket.ts b/WebApp/src/websocket.ts index b38f3d2fe..9d763ccae 100644 --- a/WebApp/src/websocket.ts +++ b/WebApp/src/websocket.ts @@ -134,13 +134,13 @@ export default class WSSignaling { let newOffer = new Offer(message.sdp, Date.now(), false); if (this.isPrivate) { - const pair = connectionPair.get(connectionId); - const otherSessionWs = pair[0] == ws ? pair[1] : pair[0]; - if (otherSessionWs) { - newOffer.polite = true; - otherSessionWs.send(JSON.stringify({ from: connectionId, to: "", type: "offer", data: newOffer })); - } else { - ws.send(JSON.stringify({ type: "error", message: `${connectionId}: This connection id is not ready other session.` })); + if (connectionPair.has(connectionId)) { + const pair = connectionPair.get(connectionId); + const otherSessionWs = pair[0] == ws ? pair[1] : pair[0]; + if (otherSessionWs) { + newOffer.polite = true; + otherSessionWs.send(JSON.stringify({ from: connectionId, to: "", type: "offer", data: newOffer })); + } } return; } @@ -160,28 +160,18 @@ export default class WSSignaling { connectionIds.add(connectionId); const newAnswer = new Answer(message.sdp, Date.now()); - let otherSessionWs = null; - - if (this.isPrivate) { - const pair = connectionPair.get(connectionId); - otherSessionWs = pair[0] == ws ? pair[1] : pair[0]; - } else { - const pair = connectionPair.get(connectionId); - otherSessionWs = pair[0]; - connectionPair.set(connectionId, [otherSessionWs, ws]); + if (!connectionPair.has(connectionId)) { + return; } - if (this.isPrivate) { - otherSessionWs.send(JSON.stringify({ from: connectionId, to: "", type: "answer", data: newAnswer })); - return; + const pair = connectionPair.get(connectionId); + const otherSessionWs = pair[0] == ws ? pair[1] : pair[0]; + + if (!this.isPrivate) { + connectionPair.set(connectionId, [otherSessionWs, ws]); } - clients.forEach((_v, k) => { - if (k == ws) { - return; - } - k.send(JSON.stringify({ from: connectionId, to: "", type: "answer", data: newAnswer })); - }); + otherSessionWs.send(JSON.stringify({ from: connectionId, to: "", type: "answer", data: newAnswer })); } private onCandidate(ws: WebSocket, message: any) { @@ -189,10 +179,12 @@ export default class WSSignaling { const candidate = new Candidate(message.candidate, message.sdpMLineIndex, message.sdpMid, Date.now()); if (this.isPrivate) { - const pair = connectionPair.get(connectionId); - const otherSessionWs = pair[0] == ws ? pair[1] : pair[0]; - if (otherSessionWs) { - otherSessionWs.send(JSON.stringify({ from: connectionId, to: "", type: "candidate", data: candidate })); + if (connectionPair.has(connectionId)) { + const pair = connectionPair.get(connectionId); + const otherSessionWs = pair[0] == ws ? pair[1] : pair[0]; + if (otherSessionWs) { + otherSessionWs.send(JSON.stringify({ from: connectionId, to: "", type: "candidate", data: candidate })); + } } return; } diff --git a/com.unity.renderstreaming/Runtime/Scripts/RenderStreamingInternal.cs b/com.unity.renderstreaming/Runtime/Scripts/RenderStreamingInternal.cs index 2df99363e..d1729ac4f 100644 --- a/com.unity.renderstreaming/Runtime/Scripts/RenderStreamingInternal.cs +++ b/com.unity.renderstreaming/Runtime/Scripts/RenderStreamingInternal.cs @@ -396,13 +396,18 @@ PeerConnection CreatePeerConnection(string connectionId, bool polite) { onAddReceiver?.Invoke(connectionId, trackEvent.Receiver); }; - pc.OnNegotiationNeeded = () => OnNegotiationNeeded(connectionId); + pc.OnNegotiationNeeded = () => _startCoroutine(OnNegotiationNeeded(connectionId)); return peer; } void DeletePeerConnection(string connectionId) { - _mapConnectionIdAndPeer[connectionId].Dispose(); + if (!_mapConnectionIdAndPeer.TryGetValue(connectionId, out var peer)) + { + return; + } + + peer.Dispose(); _mapConnectionIdAndPeer.Remove(connectionId); } @@ -424,15 +429,16 @@ void OnIceConnectionChange(string connectionId, RTCIceConnectionState state) } } - void OnNegotiationNeeded(string connectionId) + IEnumerator OnNegotiationNeeded(string connectionId) { + yield return new WaitWhile(() => !IsStable(connectionId)); SendOffer(connectionId); } IEnumerator SendOfferCoroutine(string connectionId, PeerConnection pc) { // waiting other setLocalDescription process - yield return new WaitWhile(() => pc.makingOffer || pc.makingAnswer); + yield return new WaitWhile(() => !IsStable(connectionId)); Assert.AreEqual(pc.peer.SignalingState, RTCSignalingState.Stable, $"{pc} negotiationneeded always fires in stable state"); diff --git a/com.unity.renderstreaming/Runtime/Scripts/Signaling/HttpSignaling.cs b/com.unity.renderstreaming/Runtime/Scripts/Signaling/HttpSignaling.cs index af3cebbf4..b3daeb8d8 100644 --- a/com.unity.renderstreaming/Runtime/Scripts/Signaling/HttpSignaling.cs +++ b/com.unity.renderstreaming/Runtime/Scripts/Signaling/HttpSignaling.cs @@ -150,8 +150,9 @@ private void HTTPPooling() try { HTTPGetConnections(); - HTTPGetOffers(); + //ToDo workaround: The processing order needs to be determined by the time stamp HTTPGetAnswers(); + HTTPGetOffers(); HTTPGetCandidates(); Thread.Sleep((int)(m_timeout * 1000)); diff --git a/com.unity.renderstreaming/Tests/Runtime/PrivateSignalingTest.cs b/com.unity.renderstreaming/Tests/Runtime/PrivateSignalingTest.cs index 73ce2b63d..409186665 100644 --- a/com.unity.renderstreaming/Tests/Runtime/PrivateSignalingTest.cs +++ b/com.unity.renderstreaming/Tests/Runtime/PrivateSignalingTest.cs @@ -288,12 +288,6 @@ public IEnumerator OnOffer() yield return new WaitUntil(() => !string.IsNullOrEmpty(connectionId1)); signaling2.OnOffer += (s, e) => { offerRaised2 = true; }; - - if (m_SignalingType != typeof(MockSignaling)) - { - LogAssert.Expect(LogType.Error, new Regex(".")); - } - signaling1.SendOffer(connectionId, m_DescOffer); yield return new WaitForSeconds(3); // Do not receive offer other signaling if not connected same sendoffer connectionId in private mode diff --git a/com.unity.renderstreaming/Tests/Runtime/RenderStreamingInternalTest.cs b/com.unity.renderstreaming/Tests/Runtime/RenderStreamingInternalTest.cs index 09232f724..d586fa79e 100644 --- a/com.unity.renderstreaming/Tests/Runtime/RenderStreamingInternalTest.cs +++ b/com.unity.renderstreaming/Tests/Runtime/RenderStreamingInternalTest.cs @@ -159,7 +159,7 @@ public IEnumerator OpenConnectionThrowException(TestMode mode) [TestCase(TestMode.PublicMode, ExpectedResult = null)] [TestCase(TestMode.PrivateMode, ExpectedResult = null)] [UnityTest, Timeout(10000)] - [UnityPlatform(exclude = new[] { RuntimePlatform.LinuxPlayer})] + [UnityPlatform(exclude = new[] {RuntimePlatform.LinuxPlayer})] public IEnumerator AddTrack(TestMode mode) { MockSignaling.Reset(mode == TestMode.PrivateMode); @@ -237,7 +237,7 @@ public IEnumerator AddTrackThrowException(TestMode mode) [TestCase(TestMode.PublicMode, ExpectedResult = null)] [TestCase(TestMode.PrivateMode, ExpectedResult = null)] [UnityTest, Timeout(10000)] - [UnityPlatform(exclude = new[] { RuntimePlatform.LinuxPlayer})] + [UnityPlatform(exclude = new[] {RuntimePlatform.LinuxPlayer})] public IEnumerator AddTrackMultiple(TestMode mode) { MockSignaling.Reset(mode == TestMode.PrivateMode); @@ -318,7 +318,7 @@ public IEnumerator CreateChannel(TestMode mode) //todo:: crash in dispose process on standalone linux [UnityTest, Timeout(10000)] - [UnityPlatform(exclude = new[] { RuntimePlatform.LinuxPlayer})] + [UnityPlatform(exclude = new[] {RuntimePlatform.LinuxPlayer})] public IEnumerator OnAddReceiverPrivateMode() { MockSignaling.Reset(true); @@ -391,7 +391,7 @@ public IEnumerator OnAddReceiverPrivateMode() //todo:: crash in dispose process on standalone linux [UnityTest, Timeout(10000)] - [UnityPlatform(exclude = new[] { RuntimePlatform.LinuxPlayer})] + [UnityPlatform(exclude = new[] {RuntimePlatform.LinuxPlayer})] public IEnumerator OnAddReceiverPublicMode() { MockSignaling.Reset(false); @@ -701,7 +701,82 @@ public IEnumerator ResendOfferUntilGotAnswer(TestMode mode) yield return new WaitForSeconds(ResendOfferInterval * 2); var currentCount = countGotOffer2; yield return new WaitForSeconds(ResendOfferInterval * 2); - Assert.That(countGotOffer2, Is.EqualTo(currentCount), $"{nameof(currentCount)} is not Equal {nameof(countGotOffer2)}"); + Assert.That(countGotOffer2, Is.EqualTo(currentCount), + $"{nameof(currentCount)} is not Equal {nameof(countGotOffer2)}"); + + target1.DeleteConnection(connectionId); + target2.DeleteConnection(connectionId); + + bool isDeletedConnection1 = false; + bool isDeletedConnection2 = false; + target1.onDeletedConnection += _ => { isDeletedConnection1 = true; }; + target2.onDeletedConnection += _ => { isDeletedConnection2 = true; }; + yield return new WaitUntil(() => isDeletedConnection1 && isDeletedConnection2); + Assert.That(isDeletedConnection1, Is.True, $"{nameof(isDeletedConnection1)} is not True."); + Assert.That(isDeletedConnection2, Is.True, $"{nameof(isDeletedConnection1)} is not True."); + + target1.Dispose(); + target2.Dispose(); + } + + [UnityTest, Timeout(10000)] + public IEnumerator ReNegotiationAfterReceivingFirstOffer() + { + MockSignaling.Reset(true); + + var dependencies1 = CreateDependencies(); + var dependencies2 = CreateDependencies(); + var target1 = new RenderStreamingInternal(ref dependencies1); + var target2 = new RenderStreamingInternal(ref dependencies2); + + bool isStarted1 = false; + bool isStarted2 = false; + target1.onStart += () => { isStarted1 = true; }; + target2.onStart += () => { isStarted2 = true; }; + yield return new WaitUntil(() => isStarted1 && isStarted2); + + bool isCreatedConnection1 = false; + bool isCreatedConnection2 = false; + target1.onCreatedConnection += _ => { isCreatedConnection1 = true; }; + target2.onCreatedConnection += _ => { isCreatedConnection2 = true; }; + + var connectionId = "12345"; + + // target1 has impolite peer (request first) + target1.CreateConnection(connectionId); + yield return new WaitUntil(() => isCreatedConnection1); + + // target2 has polite peer (request second) + target2.CreateConnection(connectionId); + yield return new WaitUntil(() => isCreatedConnection2); + + bool isGotOffer1 = false; + bool isGotOffer2 = false; + bool isGotAnswer1 = false; + bool isGotAnswer2 = false; + target1.onGotOffer += (_, sdp) => { isGotOffer1 = true; }; + target2.onGotOffer += (_, sdp) => { isGotOffer2 = true; }; + target1.onGotAnswer += (_, sdp) => { isGotAnswer1 = true; }; + target2.onGotAnswer += (_, sdp) => { isGotAnswer2 = true; }; + + target1.AddTransceiver(connectionId, TrackKind.Video, RTCRtpTransceiverDirection.SendOnly); + target1.AddTransceiver(connectionId, TrackKind.Video, RTCRtpTransceiverDirection.RecvOnly); + target2.AddTransceiver(connectionId, TrackKind.Video, RTCRtpTransceiverDirection.SendOnly); + target2.AddTransceiver(connectionId, TrackKind.Video, RTCRtpTransceiverDirection.RecvOnly); + + yield return new WaitUntil(() => isGotOffer2); + Assert.That(isGotOffer2, Is.True, $"{nameof(isGotOffer2)} is not True."); + target2.SendAnswer(connectionId); + + yield return new WaitUntil(() => isGotAnswer1); + Assert.That(isGotAnswer1, Is.True, $"{nameof(isGotAnswer1)} is not True."); + + yield return new WaitUntil(() => isGotOffer1); + Assert.That(isGotOffer1, Is.True, $"{nameof(isGotOffer1)} is not True."); + target1.SendAnswer(connectionId); + + yield return new WaitUntil(() => isGotAnswer2); + Assert.That(isGotAnswer2, Is.True, $"{nameof(isGotAnswer2)} is not True."); target1.DeleteConnection(connectionId); target2.DeleteConnection(connectionId); diff --git a/com.unity.renderstreaming/Tests/Runtime/Signaling/MockSignaling.cs b/com.unity.renderstreaming/Tests/Runtime/Signaling/MockSignaling.cs index 4d6f180ac..f9e1dd7e4 100644 --- a/com.unity.renderstreaming/Tests/Runtime/Signaling/MockSignaling.cs +++ b/com.unity.renderstreaming/Tests/Runtime/Signaling/MockSignaling.cs @@ -174,7 +174,7 @@ public async Task Answer(MockSignaling owner, DescData data) var list = FindList(owner, data.connectionId); if (list == null) { - Debug.LogError($"{data.connectionId} This connection id is not ready other session."); + Debug.LogWarning($"{data.connectionId} This connection id is not ready other session."); return; }