RDKEMW-24812: DefaultMessageDispatcher stops after 5-10 minutes of pl… - #144
Conversation
…ayback Reason for change: Updated polyfills for fetch and xhr Test Procedure: build should be successful. Risks: low Priority: P2
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved fetch, XHR, and runtime compatibility issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates XHR and fetch polyfills to improve playback reliability and runtime compatibility.
Changes:
- Adds XHR timeout, redirect, abort, and error handling.
- Updates fetch stream, JSON, timeout, and compression behavior.
- Adds runtime compatibility polyfills.
File summaries
| File | Summary |
|---|---|
utils/xhr.js |
XHR timeout, redirect, and lifecycle handling |
src/jsc/modules/node-fetch.js |
Fetch stream, response, JSON, timeout, and compression changes |
src/jsc/modules/linkedjsdomwrapper.js |
Runtime compatibility polyfills |
Review details
Suppressed comments (5)
src/jsc/modules/linkedjsdomwrapper.js:31
- This compatibility implementation runs for every
FetchError/AbortErrorbecause node-fetch callsError.captureStackTracein those constructors. Logging the constructor, message, type, and full stack unconditionally turns each failure into production log noise and can expose URL/query data; keep the polyfill silent or gate diagnostics behind an explicit debug flag.
if (typeof console !== 'undefined' && typeof console.log === 'function') {
var ctorName = (constructorOpt && constructorOpt.name) ? constructorOpt.name : 'unknown';
console.log('Error.captureStackTrace called for ' + ctorName + ' (polyfilled no-op, previously would have thrown and hung the fetch promise)');
src/jsc/modules/node-fetch.js:2788
- When
PassThroughis unavailable, this shortcut always useszlib.createInflate(). The normal branch below distinguishes zlib-wrapped and raw deflate by inspecting the first chunk and usescreateInflateRaw()for raw responses; this fallback makes raw-deflate servers fail in the runtime it is intended to support.
if (!hasPassThrough$1) {
body = body.pipe(zlib.createInflate());
response = new Response(body, response_options);
resolve(response);
return;
src/jsc/modules/node-fetch.js:2730
- This unconditionally calls
res.pipe()wheneverPassThroughexists, even thoughisStreamLike()explicitly supports objects that provideon/oncewithoutpipe. Such a native response reaches this line and throws before the fetch promise resolves; preserve an event-based fallback for non-pipeable responses, including the compressed-response path.
let body = hasPassThrough$1 ? res.pipe(new PassThrough$1()) : res;
utils/xhr.js:489
- The transport emits
abortfor the explicitrequest.abort()calls in bothopen()andthis.abort().didTimeoutis false on those paths, so this listener invokeshandleErrorand produces anerror/loadendafter the intended abort (also violatingopen()'s no-event reset). Track a caller-abort flag or suppress this listener before those calls.
if (didTimeout) {
return;
}
clearRequestTimeout();
self.handleError(new Error("XMLHttpRequest: request aborted by transport"));
utils/xhr.js:502
- Setting
didTimeoutonly bypasses theabortlistener above;request.on("error", errorHandler)can still receive the error emitted byrequest.abort()and callhandleError. A timeout can therefore dispatchtimeout/loadendand then dispatcherror/anotherloadend, while overwriting the timeout status. All request/response error paths need to ignore events after a timeout.
didTimeout = true;
if (request) {
request.abort();
request = null;
}
- Files reviewed: 3/3 changed files
- Comments generated: 7
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain across XHR, fetch, and runtime compatibility behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (9)
src/jsc/modules/linkedjsdomwrapper.js:58
- This replaces a standard global API and changes
Object.getOwnPropertySymbols(null)/undefinedfrom a requiredTypeErrorinto a silent empty result. Any unrelated code that relies on that validation can now continue with corrupted assumptions, while the global mutation affects all modules loaded after this wrapper. Guard the specific failing call instead of changing the built-in's semantics process-wide.
var guarded = function(obj) {
if (obj === undefined || obj === null) {
try {
if (typeof console !== 'undefined' && typeof console.log === 'function') {
console.log('Object.getOwnPropertySymbols called with ' + obj + ', returning [] instead of throwing');
src/jsc/modules/linkedjsdomwrapper.js:22
Error.captureStackTraceis installed only by this JSDOM wrapper, butnode-fetch.jscalls it when constructing everyFetchError/AbortError.JavaScriptContext.cppcan enable fetch without loading the JSDOM branch, so fetch-only contexts still throwTypeError: Error.captureStackTrace is not a functionwhile handling network or abort failures; the failure path then cannot reject cleanly. Install this compatibility shim whenever fetch is enabled, or guard the calls in node-fetch instead of limiting it to JSDOM.
if (typeof Error.captureStackTrace !== 'function') {
Error.captureStackTrace = function(targetObject, constructorOpt) {
src/jsc/modules/node-fetch.js:1613
Response/Bodynow acceptsisStreamLikebodies, but cloning still tees onlybody instanceof Stream; a cross-realm or custom stream-like body therefore falls through and is returned unchanged.Response.clone()will share the same one-shot stream instead of producing an independent clone. IncludeisStreamLike(body)in this condition.
if (body instanceof Stream && typeof body.getBoundary !== 'function') {
if (typeof PassThrough !== 'function') {
src/jsc/modules/node-fetch.js:2730
- Removing the abort listener immediately after response headers means an
AbortSignalfired while the response body is being read no longer callsabort()or emits an error onresponse.body. Fetch cancellation therefore stops working for the body-consumption phase. Keep the listener until the response body closes/errors (including decompression streams), then remove it during that cleanup.
if (signal) signal.removeEventListener('abort', abortAndFinalize);
let body = hasPassThrough$1 ? res.pipe(new PassThrough$1()) : res;
src/jsc/modules/node-fetch.js:1333
Response.json()must reject an empty or whitespace-only body because it is not valid JSON, but this branch silently converts those responses (including 204 and empty 200 responses) into{}. Callers can now accept missing or malformed payloads instead of handling the parse failure.
if (text.trim() === '') {
return {};
}
src/jsc/modules/node-fetch.js:1195
isStreamLikeaccepts objects withon/oncebut nopipe, yetwriteToStream()still unconditionally invokesbody.pipe(dest)for non-buffer bodies. Such a request body now passes Body normalization and fails only when fetch sends it; require a pipe-capable body for request uploads or add a forwarding path for non-pipe stream-likes.
function isStreamLike(body) {
return !!body
&& typeof body.on === 'function'
&& (typeof body.pipe === 'function' || typeof body.once === 'function');
}
utils/xhr.js:397
- Clearing the XHR timer as soon as headers arrive means
timeoutno longer covers a stalled response body, and it also leaves a redirected request with no timer at all. Keep the timer active until the final responseend/error, or explicitly restart it for each redirected request, so a server that sends headers and then hangs cannot leave the XHR pending indefinitely.
var responseHandler = function responseHandler(resp) {
clearRequestTimeout();
utils/xhr.js:511
- The timeout path aborts the transport and emits
timeout/loadend, but the request'serrorlistener remains installed. An abort can subsequently emit an ECONNRESET/error event, causinghandleErrorto dispatch a second terminal error/readystatechange/loadend and overwrite the timeout result. Suppress late request/response callbacks afterdidTimeout(or detach the listeners) so timeout remains the single terminal outcome.
if (this.timeout > 0) {
requestTimeoutId = setTimeout(function() {
if (!sendFlag) {
return;
}
didTimeout = true;
if (request) {
request.abort();
request = null;
}
self.status = 0;
self.statusText = "timeout";
self.responseText = "";
errorFlag = true;
sendFlag = false;
setState(self.DONE);
self.dispatchEvent("timeout");
self.dispatchEvent("loadend");
}, this.timeout);
utils/xhr.js:508
- This callback marks the XHR
DONEand clearssendFlag, but a response already queued by the native HTTP binding can still enterresponseHandler, which has no entry guard before updatingstatusor following redirects. That can overwrite the timeout result (status === 0) after the timeout event. Ignore response callbacks once the request has timed out or been aborted.
sendFlag = false;
setState(self.DONE);
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
…ayback
Reason for change: Updated polyfills for fetch and xhr
Test Procedure: build should be successful.
Risks: low
Priority: P2