From 767b921656fa54abfcc8aa5f452b3aa56eb5ae5b Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Tue, 7 Sep 2021 21:47:37 -0700 Subject: [PATCH 1/4] Make xhr transports resilient to onmessage errors Fixes #563 --- lib/transport/receiver/xhr.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/transport/receiver/xhr.js b/lib/transport/receiver/xhr.js index 8ec7bc84..f662c96f 100644 --- a/lib/transport/receiver/xhr.js +++ b/lib/transport/receiver/xhr.js @@ -37,12 +37,13 @@ XhrReceiver.prototype._chunkHandler = function(status, text) { return; } - for (var idx = -1; ; this.bufferPosition += idx + 1) { + for (var idx = -1; ; ) { var buf = text.slice(this.bufferPosition); idx = buf.indexOf('\n'); if (idx === -1) { break; } + this.bufferPosition += idx + 1; var msg = buf.slice(0, idx); if (msg) { debug('message', msg); From 082e6ddd91dd3224fb3ba94bbfb565e7e37d8254 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Wed, 22 Sep 2021 16:09:31 -0700 Subject: [PATCH 2/4] Add unit test for xhr unhandled errors fix --- tests/lib/receivers.js | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/tests/lib/receivers.js b/tests/lib/receivers.js index 823c9698..4b9e10fa 100644 --- a/tests/lib/receivers.js +++ b/tests/lib/receivers.js @@ -226,6 +226,7 @@ describe('Receivers', function () { return; } try { + expect(i).to.equal(3); expect(reason).to.equal('network'); } catch (e) { done(e); @@ -233,7 +234,7 @@ describe('Receivers', function () { } done(); }); - xhr._chunkHandler(200, 'test\nmultiple\nlines'); + xhr._chunkHandler(200, 'test\nmultiple\nlines\n'); }); it('emits no messages for an empty string response', function (done) { @@ -287,5 +288,41 @@ describe('Receivers', function () { }); xhr.abort(); }); + + it('survives errors in message handlers', function (done) { + var test = this.runnable(); + var xhr = new XhrReceiver('test', XhrFake); + var messages = []; + xhr.on('message', function (msg) { + messages.push(msg); + if (msg === 'one') { + throw new Error('boom'); + } + }); + xhr.on('close', function (code, reason) { + if (test.timedOut || test.duration) { + return; + } + try { + expect(messages).to.eql(['one', 'two', 'three']); + } catch (e) { + done(e); + return; + } + done(); + }); + try { + xhr._chunkHandler(200, 'one\n'); + } catch (e) { + // This is expected + } + try { + xhr._chunkHandler(200, 'one\ntwo\nthree\n'); + } catch (e) { + // This is NOT expected, but let the error be reported in the close handler + // instead of here + } + xhr.abort(); + }); }); }); From e10d608cc444d554de5b4e861e7c54bfcbb4084f Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Wed, 22 Sep 2021 16:21:36 -0700 Subject: [PATCH 3/4] Allow jsonp receiver to survive errors in message handlers Without this fix, once a jsonp-polling message handler throws an uncaught exception, the receiver stops polling. --- lib/transport/receiver/jsonp.js | 14 +++++++++----- tests/lib/receivers.js | 26 ++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/lib/transport/receiver/jsonp.js b/lib/transport/receiver/jsonp.js index 365fecc2..c4dae1e0 100644 --- a/lib/transport/receiver/jsonp.js +++ b/lib/transport/receiver/jsonp.js @@ -55,12 +55,16 @@ JsonpReceiver.prototype._callback = function(data) { return; } - if (data) { - debug('message', data); - this.emit('message', data); + try { + if (data) { + debug('message', data); + this.emit('message', data); + } + } finally { + // Must clean up even if 'message' event handlers throw + this.emit('close', null, 'network'); + this.removeAllListeners(); } - this.emit('close', null, 'network'); - this.removeAllListeners(); }; JsonpReceiver.prototype._abort = function(err) { diff --git a/tests/lib/receivers.js b/tests/lib/receivers.js index 4b9e10fa..8281cef5 100644 --- a/tests/lib/receivers.js +++ b/tests/lib/receivers.js @@ -55,6 +55,32 @@ describe('Receivers', function () { }); }); + it('survives errors in handlers', function (done) { + var test = this.runnable(); + JsonpReceiver.prototype._createScript = function () { + var self = this; + setTimeout(function () { + try { + global[utils.WPrefix][self.id]('datadata'); + } catch (e) { + if (!(test.timedOut || test.duration)) { + done(new Error('close event not fired')); + } + } + }, 5); + }; + var jpr = new JsonpReceiver('test'); + jpr.on('close', function () { + if (test.timedOut || test.duration) { + return; + } + done(); + }); + jpr.on('message', function () { + throw new Error('boom'); + }); + }); + it('will timeout', function (done) { this.timeout(500); var test = this.runnable(); From d4e5c2f6e567f0c034edfd0a56cf98981a94e895 Mon Sep 17 00:00:00 2001 From: Joe Cheng Date: Wed, 22 Sep 2021 19:15:50 -0700 Subject: [PATCH 4/4] Use explicit try/finally instead of careful statement ordering --- lib/transport/receiver/xhr.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/transport/receiver/xhr.js b/lib/transport/receiver/xhr.js index f662c96f..9672c017 100644 --- a/lib/transport/receiver/xhr.js +++ b/lib/transport/receiver/xhr.js @@ -43,11 +43,17 @@ XhrReceiver.prototype._chunkHandler = function(status, text) { if (idx === -1) { break; } - this.bufferPosition += idx + 1; var msg = buf.slice(0, idx); - if (msg) { - debug('message', msg); - this.emit('message', msg); + try { + if (msg) { + debug('message', msg); + this.emit('message', msg); + } + } finally { + // The bufferPosition must advance even if 'message' handler + // throws an exception, otherwise the same message will be + // replayed the next time _chunkHandler is called. + this.bufferPosition += idx + 1; } } };