tls: avoid throw in onerror for bad TLSSocket obj

[sigv]

TLSWrap.onerror has a helpful debug() call built in to it. However in case of a malformed TLSSocket object, where the _tlsOptions value is an unexpected undefined, accessing _tlsOptions.isServer causes a TypeError to be thrown.

This commit ensures that the debug() call properly logs the state as 'unknown', instead of the two 'server' and 'client' choices previously available. Additionally, onerror branching is adjusted to allow such undefined options object, by use of optional chaining.

Other methods are not being adjusted, as such a case of undefined options is not viable during regular processing of the TLSSocket.

Fixes: #41501

[sigv]

Force-push to resolve failure in commit message check:

 Error: not ok 1 fixes-url: Fixes must be a GitHub URL. (nodejs/node#41501)

[sigv]

Force-push to resolve failure in indent lint (2-space, instead of 4-space):

/home/runner/work/node/node/lib/_tls_wrap.js
Error:   412:1  error  Expected indentation of 10 spaces but found 12  indent
Error:   413:1  error  Expected indentation of 10 spaces but found 12  indent

[lpinca]

Is there a way to test this? How can owner._tlsOptions be undefined? I think this is masking a hidden bug.

[sigv]

Yeah I do agree with this being a hidden bug, as I wrote in #41501 but it's hard for me to figure out what the bug is, as whatever the network connection fails with gets masked by this TypeError during onerror() handling. So I would prefer such a safeguard to be built in, instead of having to rebuild Node locally with such a change, and without having to break out in-depth debugging tools on my environment.

I am not proposing to add such protections to other handlers for TLSSocket - those can fail with undefined object, but during onerror() it would be nice to avoid additional errors that end up masking the initial error.

[sigv]

The behavior we are seeing is very strange.

I have found some people complaining about such an error in some places around the internet, with pretty much every one of those resulting in a "just fix your internet". But it's hard for me to figure out what exactly goes wrong. I don't have a clean repro case myself, just that we see these errors pop up here and there once every few weeks in our end-to-end testing. Then they disappear as magically as they appeared. If I try make some curl invocations by hand from same node, or use other environments, I do not see any problem.

So, I would like to see the internal errors that onerror() tries to deal with.. before it hits Cannot read properties of undefined (reading 'isServer').

[lpinca]

I understand but merging this in the main repo would make it even harder to debug. Using a locally patched Node.js version to understand what is going on seems a better option.

[sigv]

How does it become harder to debug? If an error already happened, connection's already been closed as onerror() deals with it. If the offending error is the first one, it will be thrown as part of the standard onerror() handling, which results in us seeing the exact error that took place. Without the change, all I see is a TypeErrror, where _tlsOptions was undefined.

I can try rebuilding this locally, but why I would like to avoid it is we have seen this intermittently and I would result in various rebuilds for which I don't have handy infrastructure. However if you wholeheartedly believe this PR results in a degradation of the core codebase via this undefined check, then of course I can try pursuing that avenue instead.

[lpinca]

You would still need to wait (at least) until the next release, then if you find the bug you might came back and try to fix it reverting the masking code. Otherwise this code would remain here indefinitely without an explanation. I'm not very happy with this.

[sigv]

I would argue that leaving it in for onerror() is reasonable, as the onerror() should have safeguards for all values it tries to work with. Would comments explaining the reason for allowing it to be undefined make sense? Or is that still a no?

[sigv]

And to expand on it: The intention of this PR was not to push something just to have it reverted later. As I personally believe it to be helpful, in ensuring the original error always is thrown as-is, with the original stacktrace (e.g. imagine a method handleNewConnection() tries to work with _tlsOptions but fails, so we see that it was actually an error from handleNewConnection).

[lpinca]

My point is that that code was designed with the assumption that owner._tlsOptions is an object when the function is called. If that is not the case, then the process should crash as it breaks the assumption, so in my opinion we should understand why the assumption breaks and fix that without adding code to prevent the process from crashing.

I understand that reading the debug message might help but again, in my opinion it is not something we should push in the main repo and in an official release as a fix because it does not fix the actual bug.

I'm not comfortable merging this, but I will not block it if others thinks it is reasonable.

[lpinca]

@sigv in #41501 I see domainThrown: true in the thrown error. Are you using the domain module? If so can you try to not use it and see if you get the same error?

[sigv]

I see domainThrown: true in the thrown error. Are you using the domain module?

The networking issue we see gets triggered on some of our test pipelines. We use TestCafe framework for automated end-to-end web UI tests. The relevant domain usage should therefore be in src/request-pipeline/connection-reset-guard.ts from DevExpress/testcafe-hammerhead. The logic doesn't appear too convoluted as to cause issues on its own though.

If so can you try to not use it and see if you get the same error?

We have seen the issue triggered with axios, which does not appear to use domain module. We have not attempted an investigation with native https module directly as we are trying to narrow down the repro case on our side, but it's really a situation of "it comes and it goes".

People are mentioning this issue online as well. For example,

• npm install [1], [2]
• npx create-react-app [3], [4]
• yarn install [5]
• other network requests.

All of these threads, if/when resolved, conclude with "switching to mobile hotspot/home wifi/other network resolved the issue" or "the issue disappeared by itself". We are observing the same behavior in our test cases. And potentially, this issue that we are seeing is the same as everyone else is seeing, however we can't know for use unless we (1) rebuild locally with the proposed change as to see the underlying error for clues as to dig deeper or (2) we add in guarding for the undefined owner._tlsOptions when accessing isServer to the generally available release.

With this PR added, we would expect all such reports as shown in #41501 to result in more specific error messages explaining the actual issue with networking, based on the Error as passed in to onerror().

[lpinca]

Ok, please report back when this lands in a release.

[xairoo]

We have seen the issue triggered with axios, which does not appear to use domain module. We have not attempted an investigation with native https module directly as we are trying to narrow down the repro case on our side, but it's really a situation of "it comes and it goes".

I can confirm this. This issue is so random.
I am using axios to crawl many sites (many parallel requests). Currently only the top 1000 sites, just for developing.

Trying to find a host that triggers the issue didn't worked (testing since a 4 days in the background). I was only able to limit it to a range of 50 hosts within the list, but it happened only to one workstation and not every time.
Splitting the 50 hosts half and half didn't trigger the issue till now. Sometimes it happens within a few minutes, but it could be also 1 hour+.
Both workstation have the same Node.js version (v16.11.1) and both run the latest Windows 10 version. Both are connected using a cable and I am using a 10 Gbit provider. So...

[xairoo]

Is there an ETA for this PR? Would be great to get this change in a release.

[sigv]

@lpinca, are there any blockers, or anything that we can do to try get the ball rolling for releases as external people?

[lpinca]

@sigv no, any collaborator can merge this. I'm not doing it personally because I'm not comfortable with this as written above.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[sigv]

The CI failure on node-test-binary-arm-12+ > #14459 > 3,pi2-docker doesn't seem clear to me.

The parallel/test-crypto-binary-default tests failed with a timeout on it, but the only references to tls whatsoever are tls.createSecureContext calls, so I don't see how the error handling change in this PR is relevant. This passed for 3,pi3-docker, in 54ms. Is this some timing/noisy neighbor resource availability issue for test runners?

[sigv]

Then again, right before that, the previous run node-test-binary-arm-12+ > #14448 > 3,pi2-docker failed the same way. Unsure.

[nodejs-github-bot]

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

[sigv]

Seems re-run passed, so my suspicion about noisy neighbors might be correct. Thank you. 🙏🏻

@jasnell or @benjamingr, considering you did a LGTM, would it be possible for one to sponsor this change further? 🤔

[benjamingr]

@sigv sure let me add the commit-queue label which should land this!

Sorry for the slow response! We try to have better/faster iterations (especially with first time committers) and this one got missed unfortunately.

(fwiw I do also mostly agree with @lpinca that node shouldn't generally fix these sort of issues but that's a separate discussion)

[nodejs-github-bot]

Landed in a987ba1

[benjamingr]

Congrats on your first pull request landing @sigv :)

[sigv]

Thank you @benjamingr for sponsoring!

---

node shouldn't generally fix these sort of issues but that's a separate discussion

I do agree, generally code that breaks expectations should be fixed instead. For this change, however, it's hardening of error handling code.

I would like to explicitly clarify here, do you believe this change is reasonable to leave in the codebase, protecting against a null or undefined value in error handling? Or do you believe it should be rolled back later down the line, when the underlying bug is resolved? 😕

[benjamingr]

If I didn't believe this specific piece of code should be added I wouldn't have approved it or landed it :)

I do however also agree that we should figure out why ._tlsOptions is undefined in the first place and fix the underlying issue (at which point and only after we fix it we should revert this IMO).

I just don't think I prefer leaving the (problematic) behavior in the code because this fix is not a fix for the root issue.

[sigv]

Due to recent organizational change for my company, upgrading our testing cases from Node.js LTS (16.x) to Node.js Current (17.x) to investigate the TLSSocket issue is blocked. With regret, I must inform I will not be able to report further on this issue myself. But considering the number of reports I found online, I do believe someone should be able to continue - to report the underlying error that tries to bubble up.

@xairoo, if you are able to have any follow-up on this, as a new issue, it would be great. 🙏🏻

[fragglebob]

I think I've found the underlying cause the error looking into an issue in cypress-io/cypress#22291

It was introduced in this PR #38468 which started changed the owner of the TLSWrap to something that wasn't a TLSSocket and therefore missing _tlsOptions. It happened when the http Agent was reusing sockets.

That change was later reverted in v16.14.0.