HTTP Date header time faking fails on Node 15.2.0 and later

[gorankarlic]

HTTP Date header time is faked just fine up to Node v15.1.0 but fails with Node v15.2.0 or later.

• FakeTimers version : 6.0.1
• Environment : Node v15.5.0
• Other libraries you are using: assert, http

What did you expect to happen?

Should return HTTP Date header with fake time date: "Tue, 31 Dec 2013 23:59:59 GMT".

What actually happens

Instead returns HTTP Date header with current time, e.g. date: "Sun, 03 Jan 2021 17:22:52 GMT".

How to reproduce

Following is a minimal and complete mocha test case that reproduces the issue

import assert from "assert";
import http from "http";
import fake_timers from "@sinonjs/fake-timers";

describe("fake HTTP Date header time", function()
{
    let clock;
    let server;

    before(function(done)
    {
        clock = fake_timers.install({now: Date.parse("2013-12-31 23:59:59.123Z")});
        server = http.createServer((req, res) =>
        {
            req.on("data", () => void null);
            req.on("end", () =>
            {
                res.writeHead(200);
                res.end();
            });
        });
        server.listen(8080, done);
    });

    after(function(done)
    {
        clock.uninstall();
        server.close(done);
    });

    it("should fake HTTP Date header time", function(done)
    {
        const expectedHeaders =
        {
            "connection": "close",
            "date": "Tue, 31 Dec 2013 23:59:59 GMT",
            "transfer-encoding": "chunked"
        };
        const opts =
        {
            hostname: "localhost",
            method: "GET",
            path: "/",
            port: 8080
        };
        const req = http.request(opts, (res) =>
        {
            const {headers, statusCode, statusMessage} = res;
            res.on("data", () => void null);
            res.on("end", () =>
            {
                assert.strictEqual(statusCode, 200);
                assert.strictEqual(statusMessage, "OK");
                assert.deepStrictEqual(headers, expectedHeaders);
                done();
            });
        });
        req.end();
    });
});

Note

Starting with Node 15.2.0 http.js imports Date from primordials while Node 15.1.0 http.js just uses the global Date object.

[benjamingr]

That sounds reasonable, I think the current behaviour is expected but the use-case wasn't considered and the breakage wasn't expected.

Alternatives I can think of:

• Expose a --no-primordials flag or similar making primordials not return frozen instances.
• Expose an API for manipulating the base date/time inside Node.js
• Not support the case and expect users to mock time at a system level or to set the header manually themselves regardless of whether or not thy're in a test.
• Expose an API directly for http that sets the server time used in headers

[gorankarlic]

Looking at the alternatives:

1. Expose a --no-primordials flag or similar making primordials not return frozen instances.
   • Pro: simple fix that would allow stubbing of primordials across the board
   • Con: extra complexity in Node core, different production / test environments, may introduce subtleties
2. Expose an API for manipulating the base date/time inside Node.js
   • Pro: addresses the issue specifically
   • Con: does not enable stubbing of primordials in general
3. Not support the case and expect users to mock time at a system level or to set the header manually themselves regardless of whether or not thy're in a test.
   • Pro: no change Node, only need to amend Sinon documentation for clarity
   • Con: does not address actual issue; system level time mocking is not practical (messes up calendar reminders, leaves incorrect system time in case of test breakages, NTP clock-sync interference, timezone issues etc.)
4. Expose an API directly for http that sets the server time used in headers
   • Pro: Node already provides response.sendDate
   • Con: need to carry test-related code in implementation, e.g. response.sendDate = process.env.NODE_ENV === "production" or similar

I would be in favour of (1) or (2) because (3) is not practical / does not address the issue and (4) is not quite good as implementation needs to carry test code.

Looking forward to reading your opinion on this.

[benjamingr]

Hmm, @aduh95 what do you think?

[aduh95]

I quite like the flag idea as it gives to users the more power over how Node.js internals behave. What about a --expose-primordials flag? I think it would be easier to implement than a --no-primordials one, and would allow users to define a specific behaviour for Node.js internals without even changing the globals.

[benjamingr]

@gorankarlic Antoine's PR landed in Node - can you please take a look :)?

[fatso83]

The PR was merged as part of Node 15.7. What's the next step to get this closed?

[fatso83]

@gorankarlic Could you test this on Node 15.7 and see if we are getting closer?

[fatso83]

Problem not seen in:

• Node 10
• Node 12
• Node 14.15.4

Problem seen in:

• Node 14.18.0
• Node 15.14.0

I now tried to see if I could understand how to expose the internals, but I was not successful:

node --expose-internals -r internal/test/binding  node_modules/.bin/mocha breaks.js

The breaks.js file is the reproducible snippet given in the original issue. The -r internal/test/binding bit is just something I took from the nodejs/node#36872 PR, which I hoped would somehow make all of this work 🤷

[benjamingr]

I think the point of -r internal/test/binding is that it exposes a primordials global object with access to all the primordials (the locked object) - in our case we would need to check (and mock) globalThis.primordials.Date presumably.

[fatso83]

That was more promising. Thanks, Ben.

$ node --expose-internals -r internal/test/binding  -p -e 'Object.getOwnPropertyDescriptor(global.primordials, "Date")'
{
  value: [Function: Date],
  writable: false,
  enumerable: true,
  configurable: false
}
(node:1726) internal/test/binding: These APIs are for internal testing only. Do not use them.
(Use `node --trace-warnings ...` to show where the warning was created)

But ... we cannot really do much about this prop, can we? Seeing that it's neither writable nor configurable.

[benjamingr]

The primordial itself is not but its properties are. Nothing is stopping us from doing this for example:

➜  Code git:(main) node --expose-internals -r internal/test/binding -e "primordials.Date.now = () => 3; console.log(Date.now())" 
3
(node:27563) internal/test/binding: These APIs are for internal testing only. Do not use them.
(Use `node --trace-warnings ...` to show where the warning was created)