http: check "parser" existence in various conditions for http client

[BeniCheni]

Fixes: #26404 and Refs: #26351

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

After analyzing the comments, the fix and the test in #26402 for http server, got inspired to try out a similar fix and a test as to check for the parserexistence in various conditions for http client.

My local test passed in repl with similar script from #26404:

'use strict';

const http = require('http');

const server = http.createServer((req, res) => {
  res.writeHead(200, { 'Content-Type': 'text/plain' });
  res.end('okay');
});

server.listen(1337, '127.0.0.1');

const req = http.request({
  port: 1337,
  host: '127.0.0.1',
  method: 'GET',
});

req.write('');
req.end(() => { delete req.socket.parser; });

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/21292/

[BeniCheni]

Hi! What's the reason for changing this test?

Hi @Trott, that was my mistake to change the http server test in an unintended commit. I've reverted. (Sorry about that.)

Changing to parser != null would work also and be a bit safer

Per #26402 (comment) ☝️ , if if (parse != null) is suggested to be a bit safer than just if (parse), please let me know. I'm happy to make another update. Current, I'm just trying to keep the parse logic consistent to the http servion in #26402.

[lpinca]

I think this does not make sense. Any property could be deleted at any time and not only in the http module.

[Trott]

I think this does not make sense. Any property could be deleted at any time and not only in the http module.

@lpinca Deleting this property causes an assertion error. If this is not the right fix, then what is? Seems like obvious options here would be:

• Silently do nothing. In other words, skip all the things you'd usually do on the socket. This seems like a bad option, but a valid one.
• Throw an error.
• The assertion should be removed so the code fails in another way, probably throwing an error so kind of a variation on the previous option.

Something else?

@BeniCheni Can you put the var -> let/const stuff in a separate commit? (Separate PR would be even better!) These things should not be tied together as the var stuff will likely have problems being backported, but the fix should land cleanly. If they're in separate commits, at least we can just pick the one with the fix for backporting.

[lpinca]

@Trott I think that's the reason why the assertion is there in the first place. It is expected that the asserted condition holds true. Otherwise preconditions are not met and execution must be aborted.

That's the whole point of adding assertions no?

[BeniCheni]

Can you put the var -> let/const stuff in a separate commit? (Separate PR would be even better!)

@Trott, I've reverted to punt let/const changes out of this PR. I'll open a separate PR for those let/const variables. For the http client v.s. deleting a property, I'll look out the agreed option on this thread, and make the update accordingly to this PR. Thanks.

[Trott]

That's the whole point of adding assertions no?

I would say "no". An assertion firing means something that the programmer thinks can not possibly happen is happening. Otherwise, it should be some other kind of thrown error.

Assertion errors should be impossible. When they happen, it's an indication of a Node.js bug.

[Trott]

I would say "no". An assertion firing means something that the programmer thinks can not possibly happen is happening. Otherwise, it should be some other kind of thrown error.

Assertion errors should be impossible. When they happen, it's an indication of a Node.js bug.

To elaborate a bit more: If the assertion is going to stay as is, that's cool, but that means we need a code change someplace else.

[lpinca]

I agree, if it happens due to a bug in core then of course if must be fixed, but If it happens because people randomly delete properties of core objects or replace them with their own stuff, then there is nothing to do in my opinion.

[lpinca]

Here are some other examples of assertions that could throw if the developer messes with core objects:

node/lib/net.js

Line 782 in 2546351

assert(self.connecting);

node/lib/_http_outgoing.js

Line 752 in 2546351

assert(this.connection);

node/lib/internal/child_process.js

Line 663 in 2546351

assert(this.connected || this.channel);

[Trott]

I agree, if it happens due to a bug in core then of course if must be fixed, but If it happens because people randomly delete properties of core objects or replace them with their own stuff, then there is nothing to do in my opinion.

If users can cause an assertion error, the bug is in Node.js, not the user's code. User code can cause all kinds of errors, but assertion errors should never happen under any circumstances.

https://en.wikipedia.org/wiki/Assertion_(software_development):

In computer programming, an assertion is a statement that a predicate (Boolean-valued function, i.e. a true–false expression) is always true at that point in code execution. It can help a programmer read the code, help a compiler compile it, or help the program detect its own defects.

For the latter, some programs check assertions by actually evaluating the predicate as they run and if it is not in fact true, an assertion failure, the program considers itself to be broken and typically deliberately crashes or throws an assertion failure exception.

Users deleting properties can cause all sorts of errors, but they should never be able to cause an assertion error.

[lpinca]

Users deleting properties can cause all sorts of errors, but they should never be able to cause an assertion error.

We are in disagreement here. If user code changes internals in a way that makes the assertion predicate evaluate to false I can't see why the assertion error should not be thrown.

[Trott]

Here are some other examples of assertions that could throw if the developer messes with core objects:

Then each of those assertions is wrong. Assertions document things that are believed to be logically impossible. Those should be regular errors or else we should protect against such monkey-patching.

I don't want to be too stubborn or argumentative about this, and I think I've made my view clear, so I'll stop at this point. If others agree that this is a non-issue and should not be changed/fixed, we can close the PR and associated issue. (I would be surprised, though!)

[lpinca]

we should protect against such monkey-patching

this, probably, but is it viable for legacy code?

[BridgeAR]

we should protect against such monkey-patching

That would be ideal but in reality that is pretty much impossible to achieve fully. Any function could e.g. be called by users with a different scope by using .call() or .apply() and replace internal properties (this could be addressed on the long term by making internal properties really internal). There are a couple public properties which should be writable internally but only readable for the users. In such cases we might replace the functionality which a getter/setter but it will be hard and a lot of work and a very long process to reach that.

[BeniCheni]

Closing this PR as the current consensus in above comments. Will look out any further discussion in #26404.