Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Ensure that each request can only fail once. #28

Merged
merged 5 commits into from
May 22, 2024
Merged

fix: Ensure that each request can only fail once. #28

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
291fb29
fix: Ensure that each request can only fail once.
kinyoklion May 21, 2024
c9084f1
Merge branch 'main' into rlamb/request-fail-only-once
kinyoklion May 21, 2024
d762ba8
Destroy request on timeout.
kinyoklion May 22, 2024
7828a7e
generate polyfill
kinyoklion May 22, 2024
cdf2774
Remove session from debugging.
kinyoklion May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 71 additions & 52 deletions example/eventsource-polyfill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,27 +67,27 @@
/* 0 */
/***/ (function(module, exports) {

var g;

// This works in non-strict mode
g = (function() {
return this;
})();

try {
// This works if eval is allowed (see CSP)
g = g || Function("return this")() || (1,eval)("this");
} catch(e) {
// This works if the window reference is available
if(typeof window === "object")
g = window;
}

// g can still be undefined, but nothing to do about it...
// We return undefined, instead of nothing here, so it's
// easier to handle this case. if(!global) { ...}

module.exports = g;
var g;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just formatting or was there a change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is auto-generated based on the other files. I am not sure the cause of the diff here. It looks like most of the formatting is even the same.

// This works in non-strict mode
g = (function() {
return this;
})();
try {
// This works if eval is allowed (see CSP)
g = g || Function("return this")() || (1,eval)("this");
} catch(e) {
// This works if the window reference is available
if(typeof window === "object")
g = window;
}
// g can still be undefined, but nothing to do about it...
// We return undefined, instead of nothing here, so it's
// easier to handle this case. if(!global) { ...}
module.exports = g;


/***/ }),
Expand Down Expand Up @@ -4675,7 +4675,7 @@ var IncomingMessage = exports.IncomingMessage = function (xhr, response, mode, f
self.url = response.url
self.statusCode = response.status
self.statusMessage = response.statusText

response.headers.forEach(function (header, key){
self.headers[key.toLowerCase()] = header
self.rawHeaders.push(key, header)
Expand Down Expand Up @@ -4805,7 +4805,7 @@ IncomingMessage.prototype._onXHRProgress = function () {
self.push(new Buffer(response))
break
}
// Falls through in IE8
// Falls through in IE8
case 'text':
try { // This will fail when readyState = 3 in IE9. Switch mode and wait for readyState = 4
response = xhr.responseText
Expand Down Expand Up @@ -7264,6 +7264,19 @@ function hasBom (buf) {
})
}

/**
* Wrap a callback to ensure it can only be called once.
*/
function once(cb) {
let called = false;
return (...params) => {
if(!called) {
called = true;
cb(...params);
}
};
}

/**
* Creates a new EventSource object
*
Expand All @@ -7289,6 +7302,7 @@ function EventSource (url, eventSourceInitDict) {

var self = this
self.reconnectInterval = 1000
self.session = 0

var req
var lastEventId = ''
Expand Down Expand Up @@ -7428,12 +7442,17 @@ function EventSource (url, eventSourceInitDict) {
var isSecure = urlAndOptions.options.protocol === 'https:' ||
(urlAndOptions.url && urlAndOptions.url.startsWith('https:'))

self.session = self.session + 1

// Each request should be able to fail at most once.
const failOnce = once(failed);

var callback = function (res) {
// Handle HTTP redirects
if (res.statusCode === 301 || res.statusCode === 307) {
if (!res.headers.location) {
// Server sent redirect response without Location header.
failed({ status: res.statusCode, message: res.statusMessage })
failOnce({ status: res.statusCode, message: res.statusMessage })
return
}
if (res.statusCode === 307) reconnectUrl = url
Expand All @@ -7444,7 +7463,7 @@ function EventSource (url, eventSourceInitDict) {

// Handle HTTP errors
if (res.statusCode !== 200) {
failed({ status: res.statusCode, message: res.statusMessage })
failOnce({ status: res.statusCode, message: res.statusMessage })
return
}

Expand All @@ -7456,13 +7475,13 @@ function EventSource (url, eventSourceInitDict) {
res.on('close', function () {
res.removeAllListeners('close')
res.removeAllListeners('end')
failed()
failOnce()
})

res.on('end', function () {
res.removeAllListeners('close')
res.removeAllListeners('end')
failed()
failOnce()
})
_emit(new Event('open'))

Expand Down Expand Up @@ -7561,11 +7580,11 @@ function EventSource (url, eventSourceInitDict) {
}

req.on('error', function (err) {
failed({ message: err.message })
failOnce({ message: err.message })
})

req.on('timeout', function () {
failed({ message: 'Read timeout, received no data in ' + config.readTimeoutMillis +
failOnce({ message: 'Read timeout, received no data in ' + config.readTimeoutMillis +
'ms, assuming connection is dead' })
})

Expand Down Expand Up @@ -8711,28 +8730,28 @@ module.exports = CalculateCapacity
/* 33 */
/***/ (function(module, exports) {

module.exports = function(module) {
if(!module.webpackPolyfill) {
module.deprecate = function() {};
module.paths = [];
// module.parent = undefined by default
if(!module.children) module.children = [];
Object.defineProperty(module, "loaded", {
enumerable: true,
get: function() {
return module.l;
}
});
Object.defineProperty(module, "id", {
enumerable: true,
get: function() {
return module.i;
}
});
module.webpackPolyfill = 1;
}
return module;
};
module.exports = function(module) {
if(!module.webpackPolyfill) {
module.deprecate = function() {};
module.paths = [];
// module.parent = undefined by default
if(!module.children) module.children = [];
Object.defineProperty(module, "loaded", {
enumerable: true,
get: function() {
return module.l;
}
});
Object.defineProperty(module, "id", {
enumerable: true,
get: function() {
return module.i;
}
});
module.webpackPolyfill = 1;
}
return module;
};


/***/ }),
Expand Down Expand Up @@ -12056,4 +12075,4 @@ module.exports = function isBuffer(arg) {
}

/***/ })
/******/ ]);
/******/ ]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it was manually edited before, but it generated this way.

43 changes: 35 additions & 8 deletions lib/eventsource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@ function hasBom (buf) {
})
}

/**
* Wrap a callback to ensure it can only be called once.
*/
function once(cb) {
let called = false
return (...params) => {
if(!called) {
called = true
cb(...params)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a logger util in this library? It's helpful to see logs when something is ignored.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not.

}
}

/**
* Creates a new EventSource object
*
Expand All @@ -52,6 +65,7 @@ function EventSource (url, eventSourceInitDict) {

var self = this
self.reconnectInterval = 1000
self.session = 0

var req
var lastEventId = ''
Expand Down Expand Up @@ -186,17 +200,27 @@ function EventSource (url, eventSourceInitDict) {
}, delay)
}

function destroyRequest() {
if (req.destroy) req.destroy()
if (req.xhr && req.xhr.abort) req.xhr.abort()
}

function connect () {
var urlAndOptions = makeRequestUrlAndOptions()
var isSecure = urlAndOptions.options.protocol === 'https:' ||
(urlAndOptions.url && urlAndOptions.url.startsWith('https:'))

self.session = self.session + 1

// Each request should be able to fail at most once.
const failOnce = once(failed)

var callback = function (res) {
// Handle HTTP redirects
if (res.statusCode === 301 || res.statusCode === 307) {
if (!res.headers.location) {
// Server sent redirect response without Location header.
failed({ status: res.statusCode, message: res.statusMessage })
failOnce({ status: res.statusCode, message: res.statusMessage })
return
}
if (res.statusCode === 307) reconnectUrl = url
Expand All @@ -207,7 +231,7 @@ function EventSource (url, eventSourceInitDict) {

// Handle HTTP errors
if (res.statusCode !== 200) {
failed({ status: res.statusCode, message: res.statusMessage })
failOnce({ status: res.statusCode, message: res.statusMessage })
return
}

Expand All @@ -219,13 +243,13 @@ function EventSource (url, eventSourceInitDict) {
res.on('close', function () {
res.removeAllListeners('close')
res.removeAllListeners('end')
failed()
failOnce()
})

res.on('end', function () {
res.removeAllListeners('close')
res.removeAllListeners('end')
failed()
failOnce()
})
_emit(new Event('open'))

Expand Down Expand Up @@ -324,12 +348,14 @@ function EventSource (url, eventSourceInitDict) {
}

req.on('error', function (err) {
failed({ message: err.message })
failOnce({ message: err.message })
})

req.on('timeout', function () {
failed({ message: 'Read timeout, received no data in ' + config.readTimeoutMillis +
failOnce({ message: 'Read timeout, received no data in ' + config.readTimeoutMillis +
'ms, assuming connection is dead' })
// Timeout doesn't mean that the request is cancelled, just that it has elapsed the timeout.
destroyRequest()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being more careful about destroying requests will help to ensure applications are not held open by the event source.

})

if (req.setNoDelay) req.setNoDelay(true)
Expand All @@ -347,8 +373,9 @@ function EventSource (url, eventSourceInitDict) {
this._close = function () {
if (readyState === EventSource.CLOSED) return
readyState = EventSource.CLOSED
if (req.abort) req.abort()
if (req.xhr && req.xhr.abort) req.xhr.abort()

destroyRequest()

_emit(new Event('closed'))
}

Expand Down
Loading