Skip to content

RDKEMW-24812: DefaultMessageDispatcher stops after 5-10 minutes of pl… - #144

Merged
vjain008 merged 2 commits into
developfrom
topic/RDKEMW-24812
Sep 11, 2026
Merged

vjain008 merged 2 commits into
developfrom
topic/RDKEMW-24812

Conversation

@gurpreet319

Copy link
Copy Markdown
Contributor

…ayback

Reason for change: Updated polyfills for fetch and xhr
Test Procedure: build should be successful.
Risks: low
Priority: P2

…ayback

Reason for change: Updated polyfills for fetch and xhr
Test Procedure: build should be successful.
Risks: low
Priority: P2
Copilot AI lite review requested due to automatic review settings September 11, 2026 04:58
@gurpreet319
gurpreet319 requested a review from a team as a code owner September 11, 2026 04:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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/AbortError because node-fetch calls Error.captureStackTrace in 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 PassThrough is unavailable, this shortcut always uses zlib.createInflate(). The normal branch below distinguishes zlib-wrapped and raw deflate by inspecting the first chunk and uses createInflateRaw() 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() whenever PassThrough exists, even though isStreamLike() explicitly supports objects that provide on/once without pipe. 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 abort for the explicit request.abort() calls in both open() and this.abort(). didTimeout is false on those paths, so this listener invokes handleError and produces an error/loadend after the intended abort (also violating open()'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 didTimeout only bypasses the abort listener above; request.on("error", errorHandler) can still receive the error emitted by request.abort() and call handleError. A timeout can therefore dispatch timeout/loadend and then dispatch error/another loadend, 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.

Comment thread src/jsc/modules/linkedjsdomwrapper.js
Comment thread src/jsc/modules/linkedjsdomwrapper.js
Comment thread src/jsc/modules/node-fetch.js
Comment thread src/jsc/modules/node-fetch.js
Comment thread src/jsc/modules/node-fetch.js
Comment thread src/jsc/modules/node-fetch.js
Comment thread utils/xhr.js
Copilot AI review requested due to automatic review settings September 11, 2026 05:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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)/undefined from a required TypeError into 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.captureStackTrace is installed only by this JSDOM wrapper, but node-fetch.js calls it when constructing every FetchError/AbortError. JavaScriptContext.cpp can enable fetch without loading the JSDOM branch, so fetch-only contexts still throw TypeError: Error.captureStackTrace is not a function while 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/Body now accepts isStreamLike bodies, but cloning still tees only body 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. Include isStreamLike(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 AbortSignal fired while the response body is being read no longer calls abort() or emits an error on response.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

  • isStreamLike accepts objects with on/once but no pipe, yet writeToStream() still unconditionally invokes body.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 timeout no 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 response end/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's error listener remains installed. An abort can subsequently emit an ECONNRESET/error event, causing handleError to dispatch a second terminal error/readystatechange/loadend and overwrite the timeout result. Suppress late request/response callbacks after didTimeout (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 DONE and clears sendFlag, but a response already queued by the native HTTP binding can still enter responseHandler, which has no entry guard before updating status or 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

Comment thread src/jsc/modules/linkedjsdomwrapper.js
Comment thread utils/xhr.js
@vjain008
vjain008 merged commit 58832bf into develop Sep 11, 2026
9 of 11 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 11, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants