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.

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

UND_ERR_HEADERS_TIMEOUT when changing system time during fetch #3493

Closed
IboKhaled opened this issue Aug 23, 2024 · 3 comments · Fixed by #3495
Closed

UND_ERR_HEADERS_TIMEOUT when changing system time during fetch #3493

IboKhaled opened this issue Aug 23, 2024 · 3 comments · Fixed by #3495
Labels
bug Something isn't working

Comments

@IboKhaled
Copy link

Bug Description

When the system time is changed forward by more than 5 minutes during a fetch() call, the operation terminates with UND_ERR_HEADERS_TIMEOUT.

As far as I can tell, this is due to how the timeout mechanism is implemented in lib/util/timers.js.
Timers above 1000 ms are realized with a custom, low resolution timer implementation, which relies on Date.now(). Therefore the timers are influenced by the system time. (I guess this would also mean the timeout would fire late/never when time is set back during an active fetch() operation too.)
In my testing, JS inbuilt setTimeout() is not impacted by system time changes.

Is this expected behavior?
Are there valid reasons why the timeout should be relative to the wall clock?
If this is a bug, could monotonic time functions like performance.now() or process.hrtime be used here instead?

Reproducible By

  1. Perform a long fetch operation
  2. Set system time forward by more than 5 minutes or headersTimeout
  3. Observe timeout error

Here is an unit test that verifies the early timeout for test/util.js:

test("do not timeout early on forward system time change", async (ctx) => {
  let t = tspl(ctx, { plan: 3 });

  let dateMock = ctx.mock.method(Date, "now");
  dateMock.mock.mockImplementation(() =>
    new Date("2024-08-22T10:00:00.000Z").valueOf()
  );

  const start = process.hrtime.bigint();

  timers.setTimeout(() => {
    const delta = getDelta(start, 2000);
    t.ok(delta >= -1n, "timer fired early after system time change");
    t.ok(
      delta < ACCEPTABLE_DELTA,
      "timer fired late after system time change"
    );
  }, 2000);

  // Simulate user setting time forwards with active timer
  setTimeout(
    () =>
      dateMock.mock.mockImplementation(() =>
        new Date("2024-08-22T11:00:00.000Z").valueOf()
      ),
    750
  );

  setTimeout(() => t.ok(true), 2500);
  await t.completed;
});

Expected Behavior

A fetch() calls timeout should not be influenced by system time.

Environment

Observed in Node v18.14.2, unit test written for undici on current main (69cfd97).

@IboKhaled IboKhaled added the bug Something isn't working label Aug 23, 2024
@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 23, 2024

Could you please explain, in which cases you change your system time?

@IboKhaled
Copy link
Author

I start the fetch operation, which triggers a change in the system time. This fetch operation times out because of the change in system time it itself triggered.

@ronag
Copy link
Member

ronag commented Aug 23, 2024

I think #3418 would fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants