`req.socket` being null, but external library might expect it do be defined

[sveisvei]

Bug Description

Experiencing req.socket being null: https://github.com/elastic/apm-agent-nodejs/blob/6a540301619ed21abb9f2accb8de98ba516f3531/lib/parsers.js#L23 and this code is throwing as a result.

"type": "TypeError",
"message": "Cannot read properties of null (reading 'remoteAddress')",
"stack":
    TypeError: Cannot read properties of null (reading 'remoteAddress')
        at Object.getContextFromRequest (/node_modules/.pnpm/elastic-apm-node@3.25.0/node_modules/elastic-apm-node/lib/parsers.js:28:34)
        at Transaction.toJSON (/node_modules/.pnpm/elastic-apm-node@3.25.0/node_modules/elastic-apm-node/lib/instrumentation/transaction.js:189:41)
        at Transaction._encode (/node_modules/.pnpm/elastic-apm-node@3.25.0/node_modules/elastic-apm-node/lib/instrumentation/transaction.js:218:15)
        at Instrumentation.addEndedTransaction (/node_modules/.pnpm/elastic-apm-node@3.25.0/node_modules/elastic-apm-node/lib/instrumentation/index.js:284:63)
        at Transaction.end (/node_modules/.pnpm/elastic-apm-node@3.25.0/node_modules/elastic-apm-node/lib/instrumentation/transaction.js:268:32)
        at ServerResponse.<anonymous> (/node_modules/.pnpm/elastic-apm-node@3.25.0/node_modules/elastic-apm-node/lib/instrumentation/http-shared.js:41:36)
        at ServerResponse.f (/node_modules/.pnpm/once@1.4.0/node_modules/once/once.js:25:25)
        at ServerResponse.onfinish (/node_modules/.pnpm/end-of-stream@1.4.4/node_modules/end-of-stream/index.js:31:27)
        at /node_modules/.pnpm/elastic-apm-node@3.25.0/node_modules/elastic-apm-node/lib/instrumentation/run-context/AbstractRunContextManager.js:76:49
        at AsyncHooksRunContextManager.with (/node_modules/.pnpm/elastic-apm-node@3.25.0/node_modules/elastic-apm-node/lib/instrumentation/run-context/BasicRunContextManager.js:49:17)
        at ServerResponse.wrapper (/node_modules/.pnpm/elastic-apm-node@3.25.0/node_modules/elastic-apm-node/lib/instrumentation/run-context/AbstractRunContextManager.js:76:23)
        at ServerResponse.emit (node:events:402:35)
        at ServerResponse.emit (node:domain:475:12)
        at onFinish (node:_http_outgoing:830:10)
        at afterWrite (node:internal/streams/writable:497:5)
        at afterWriteTick (node:internal/streams/writable:484:10)
        at processTicksAndRejections (node:internal/process/task_queues:82:21)

The offending line in undici, is.

undici/lib/core/util.js

Line 166 in 2917daa

stream.socket = null

Reproducible By

If e.g. https://github.com/elastic/apm-agent-nodejs hook into the request cycle after the socket has been destroyed, it will end up with a socket that is null, but this is something it does not except - and code will throw because it always expect socket to be defined.

Expected Behavior

I am not sure. I do expect us to not play with the raw socket reference in unexpected ways, or maybe ensure that we do it last.

Logs & Screenshots

Environment

16.13

Additional context

[mcollina]

I'm sorry @sveisvei but the bug is in the Elastic APM Agent, not this library.

@delvedor could you help routing this problem to whoever is working on that?

[delvedor]

Hello! This is the tracking issue for undici's instrumentation :)

elastic/apm-agent-nodejs#2383

[sveisvei]

Thank you for such a quick response.

node.js api documentation says request.socket should always be there, so nullifying will maybe break that assumption https://nodejs.org/dist/latest-v16.x/docs/api/http.html#requestsocket

[mcollina]

I'm pretty sure that will be null in a few cases (see https://github.com/nodejs/node/blob/265a47d680077945e49578f4cb0decc772ff8058/lib/_http_server.js#L250).

The doc is not correct/out of date and it has been for quite some time :(.

cc @ronag.

[ronag]

Moved to node nodejs/node#41011.

[trentm]

@sveisvei Thanks. I've opened elastic/apm-agent-nodejs#2479 to fix this. I'd love to get some more details from you there to help me reproduce, if you are able.

[sveisvei]

@trentm I can test when you have released your fix :).

The error for me happened when the remote undici was connecting with aborted a request, and did not trigger this error 100% of time as well, which is interesting.

[trentm]

@sveisvei elastic-apm-node@3.26.0 is now published (elastic/apm-agent-nodejs#2505) with the fix, if you would like to upgrade/test. Thanks.