Timeout rejections are unhandled

[moltar]

In this code:

experiment-js-client/packages/experiment-browser/src/transport/http.ts

Lines 18 to 36 in 05c2c38

	/*
	* Copied from:
	* https://github.com/github/fetch/issues/175#issuecomment-284787564
	*/
	const timeout = (
	promise: Promise<SimpleResponse>,
	timeoutMillis?: number,
	): Promise<SimpleResponse> => {
	// Don't timeout if timeout is null or invalid
	if (timeoutMillis == null || timeoutMillis <= 0) {
	return promise;
	}
	return new Promise(function (resolve, reject) {
	safeGlobal.setTimeout(function () {
	reject(Error('Request timeout after ' + timeoutMillis + ' milliseconds'));
	}, timeoutMillis);
	promise.then(resolve, reject);
	});
	};

The timeout rejections will be unhandled, polluting the console with errors, and error reporting tools such as Sentry get flooded with useless errors.

The comment on GH from which this function was copied from states for this to be safe, but indeed it is not.

A small repro is this codesandbox.

A less naive implementation can be found in this package: https://github.com/sindresorhus/p-timeout

[tyiuhc]

@moltar, many thanks for raising this issue. To reduce error-log pollution, the specific timeout errors are now logged at the debug-level in v1.12.1. Please let us know if this issue persists!

[moltar]

I don't think it addresses the main problem - which is unhandled exceptions due to naive handling of the promises.

Please refer to my repro example.

[bgiori]

Hi @moltar, thanks again for submitting this issue.

Question regarding the code sandbox example you provided.

try {
  timeout(sleep(2_000), 1_000);
} catch {
  console.log("timeout");
}

Is the expected behavior that the catch statement is run?

If so, this is expected to not occur since the timeout function returns a promise. To catch the error from the promise, we either need to await the promise returned by timeout with the existing try/catch, or use a .catch() function chained to timeout(...).

The caller of request which calls timeout is set up to handle the errors from the rejected promises. Previously we were logging all these errors including the timeouts as error logs, but now we're logging them as debug.

Does this resolve the issue? Or did I miss something with respect to how the timeout function returns it's errors?

[moltar]

@bgiori You are totally right. Missed the await in the try block.

I am not yet sure if the full issue is solved.

What we have observed is that Sentry was reporting every case of timeout. I am not sure if this is because of console.error, or if this is because of the unhandled exception. In Sentry, it was reporting it as an unhandled exception.

So it's still unclear how that can happen, if it is a simple console.log vs console.debug difference.

[bgiori]

Have you updated the version you're using in prod to 1.12.1+?

If not, give that a shot. If so, a screenshot of the sentry error would be helpful. To avoid posting publicly feel free to email it to me: [email protected]

[tyiuhc]

@moltar thank you for your patience regarding this issue. It is possible that the continued Sentry errors are due to timeout errors not being caught for flag fetches when the client is not in debug-logging mode. v1.12.3 will now always catch timeout errors, regardless of logging mode. Please let us know if your issue persists.