Closed gorankarlic closed 3 months ago
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:
--no-primordials
flag or similar making primordials
not return frozen instances.http
that sets the server time used in headersLooking at the alternatives:
--no-primordials
flag or similar making primordials
not return frozen instances.
http
that sets the server time used in headers
response.sendDate = process.env.NODE_ENV === "production"
or similarI 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.
Hmm, @aduh95 what do you think?
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.
@gorankarlic Antoine's PR landed in Node - can you please take a look :)?
The PR was merged as part of Node 15.7. What's the next step to get this closed?
@gorankarlic Could you test this on Node 15.7 and see if we are getting closer?
Problem not seen in:
Problem seen in:
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 https://github.com/nodejs/node/pull/36872 PR, which I hoped would somehow make all of this work 🤷
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.
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.
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)
Yeah, but in the case of this exact issue, it seems that it is the Date constructor the code supplied by @gorankarlic depends upon, so we are kind of screwed. Not a huge loss IMHO: we can never guarantee that modules outside of our control does not hold an internal reference to the original Date object, which is what is going on here anyway.
But I think we can override the methods called after new Date()
which are called (like toUTCString() and valueOf()) instead can't we?
Aha ... true. There are more ways to skin a cat, obviously 😄 We cannot have our cake, but we can probably eat it anyway, etc, etc..
With regards to the current http implementation, just primordials.Date.prototype.toUTCString = function(){ return 'foo' }
probably won't fly. Node http
caches a direct reference to DatePrototypeToUTCString
, which I think defeats any attempt we could come up with that touches Date
.
This target seems to move faster than we can come up with workarounds, so I am suspecting that even if we should start replacing all methods on the Date.prototype
the amount of usecases we actually are able to support is rather slim. Don't mind trying, though. Or rather, I don't mind someone trying 😃
I can just make a PR to node to not do this when the --expose-primordials flag is set probably?
Let's see how it goes https://github.com/nodejs/node/pull/40733
This seems like something we cannot fix ourselves, @gorankarlic. Ref https://github.com/nodejs/node/pull/40733#issuecomment-1016522091
Thank you for trying hard to fix this & sorry for the late reply.
It seems the only way forward is to ignore the HTTP Date
header.
IMHO it's very sad that NodeJS seems to have sacrificed it's original, simple and beautiful idea - i.e. expose the POSIX API to ECMAScript - to such an extent that AFAIK it now blatantly violates ECMAScript itself, in that modifying a prototype is not possible and/or does not result in the correct/expected behaviour; it instead has seemingly become a micro-optimization-driven mess at it's very core.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This is not moving anywhere and it was clear to years ago that it will not be possible to hook into Node in any way. Tests are best written to not rely on this header, basically.
HTTP
Date
header time is faked just fine up toNode v15.1.0
but fails withNode v15.2.0
or later.6.0.1
Node v15.5.0
assert
,http
What did you expect to happen?
Should return HTTP
Date
header with fake timedate: "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
```js 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
fromprimordials
while Node 15.1.0 http.js just uses the globalDate
object.