-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
http: check "parser" existence in various conditions for http client #26486
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
http: check "parser" existence in various conditions for http client #26486
Conversation
Hi @Trott, that was my mistake to change the http server test in an unintended commit. I've reverted. (Sorry about that.)
Per #26402 (comment) ☝️ , if |
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:
Something else? @BeniCheni Can you put the |
@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? |
@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. |
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. |
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. |
Here are some other examples of assertions that could throw if the developer messes with core objects: Line 782 in 2546351
Line 752 in 2546351
node/lib/internal/child_process.js Line 663 in 2546351
|
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):
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 |
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!) |
this, probably, but is it viable for legacy code? |
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 |
Closing this PR as the current consensus in above comments. Will look out any further discussion in #26404. |
Fixes: #26404 and Refs: #26351
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAfter 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
parser
existence in various conditions for http client.My local test passed in repl with similar script from #26404: