fix(node): Use finish event instead of patching res.end

[jiangbo0216]

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Closes #8848

[Lms24]

This needs a review, just linking to the issue #8848

[jiangbo0216]

@Lms24 any update?

[Lms24]

Hi @jiangbo0216 thanks for the reminder and sorry that this took so long.

Generally, I think this looks like a valuable improvement but I'm not yet sure if this is the way we want to go. Dependencies always have a cost for us and I wanna discuss with the team first if we want to add on-finished.

Besides the dependency question , my other concern here is that

• the requestHandler is currently not express-specific (for example, our docs also reference it in connect)
• I understand that Express and GCP use on-finished themselves but can we be sure the other frameworks support it, too?

I think one argument in favour of merging this PR is that it was suggested before (#2344), as you linked in your issue and on-finished seems widely used.

Also another heads-up: If we decide to continue with this, we're gonna ask you to split this up into 2 PRs for Node and GCP. Makes reverting things easier if we have to :) But don't feel obligated to do this now. I'll let you know.

[jiangbo0216]

on-finished is lightweight, and as far as I know, on-finished supports all Node.js HTTP requests, so I understand it can be used in connect.
And, this will need to be discussed further with the team before making a decision. If it is necessary to split the pull request, please let me know. Thank you.

[Lms24]

@jiangbo0216 we discussed this internally and decided to generally go with the suggested change but instead of registering on-finished as a dependency, we'll vendor in the code. Meaning, we'll include the source code in its own file and in that file also include the original authors' license.

So we should create a packages/node/src/vendor/on-finished.ts file and add the code there. We can also remove unnecessary parts (as long as we mention this in the attribution section on top of the file). I think this should be reusable in the serverless SDK then.

Would you be interested in doing this? If not, no problem but I want to check first with you. Either way, we'll make sure to mention you in the changelog once we release this fix.

[jiangbo0216]

@jiangbo0216 we discussed this internally and decided to generally go with the suggested change but instead of registering on-finished as a dependency, we'll vendor in the code. Meaning, we'll include the source code in its own file and in that file also include the original authors' license.

So we should create a packages/node/src/vendor/on-finished.ts file and add the code there. We can also remove unnecessary parts (as long as we mention this in the attribution section on top of the file). I think this should be reusable in the serverless SDK then.

Would you be interested in doing this? If not, no problem but I want to check first with you. Either way, we'll make sure to mention you in the changelog once we release this fix.

thank you, I will give it a try. thanks for your kindness

I have a question, is the reason for "vendoring in the code" to keep the dependency clean?

Both "vendoring in the code" and "using it as a dependency" have their own advantages. It depends on how the team weighs the pros and cons of each approach.

Does this commit meet expectations?

https://github.com/getsentry/sentry-javascript/compare/develop...jiangbo0216:sentry-javascript:fix-on-finished?expand=1

@Lms24
Could you help review this when you have time? Thank you.

[Lms24]

@jiangbo0216 thanks for sticking with us!

I have a question, is the reason for "vendoring in the code" to keep the dependency clean

Yes, we like to avoid adding 3rd party dependencies whenever possible. Vendoring in the code in this case (given it's rather small) lets us apply changes or fixes more easily if necessary. You're right, there are disadvantages to this approach but it worked out quite well for us in the past.

Does this commit meet expectations?

Generally, yes! I have one request: Can you please copy the entire LICENSE files into the vendored files? Here's an example of some vendored in code from our Remix SDK. Feel free to add this commit to this PR so that we can review it properly. Thank you!

[Lms24]

I have another question: What's the difference between using onFinished as shown here vs. using res.once('finish', () => {}) as for example further down this file in line 200?

[jiangbo0216]

function isFinished(msg) {
  var socket = msg.socket;

  if (typeof msg.finished === 'boolean') {
    // OutgoingMessage
    return Boolean(msg.finished || (socket && !socket.writable));
  }

  if (typeof msg.complete === 'boolean') {
    // IncomingMessage
    return Boolean(msg.upgrade || !socket || !socket.readable || (msg.complete && !msg.readable));
  }

  // don't know
  return undefined;
}

  // finished on first message event
  eeMsg = eeSocket = first([[msg, 'end', 'finish']], onFinish);

  function onSocket(socket) {
    // remove listener
    msg.removeListener('socket', onSocket);

    if (finished) return;
    if (eeMsg !== eeSocket) return;

    // finished on first socket event
    eeSocket = first([[socket, 'error', 'close']], onFinish);
  }

I believe there is not much difference. If we are certain that res.once('finish', () => {}) will work as expected, then there is no need to introduce onFinished.

1. onFinished includes some compatibility code for older versions of Node.js.
2. Additionally, when the isFinished property of res is true, it can also trigger the callback function registered with onFinished.
3. onFinished register more event from http response object, like error, close, end, finish

above is what onFinished do

In my opinion, using res.once('finish', () => {}) is a more concise and maintainable approach.

On the other hand, using onFinished may carry more significance in certain cases.

if have any idea, we can discuss it further. If necessary, we can remove onFinished and directly use the standard event in Node.js.

[Lms24]

onFinished includes some compatibility code for older versions of Node.js.

That's the one thing that's important. Do you know what the minimum version for res.once('finish') is? I tried looking it up in the Node docs but couldn't find much. (Seems like once isn't even documented; once was introduced in 0.3.0). What we can see is that the 'finish' event was introduced in 0.3.6 so that should be no problem. Whatever we do here needs to support Node 8 and higher.

In my opinion, using res.once('finish', () => {}) is a more concise and maintainable approach.

I agree. If it works for Node 8+, I think we should use it instead of adding onFinished. If it turns out that this doesn't work, we can still come back to onFinished.

[Lms24]

@jiangbo0216 would you mind giving res.once('finish') a try?

[jiangbo0216]

ok, I agree with you

[jiangbo0216]

@Lms24
Could you help review this when you have time? Thank you.

[Lms24]

Thanks for sticking with us @jiangbo0216! Looks good to me now. I'd say we give it a try and if things don't work as expected, we revert and try onFinished.

[Lms24]

Sorry but it seems like we have to postpone releasing this. We talked about this change again and it seems like we need further testing. The main problem seems to be that for example in AWS lambda, execution freezes after res.end. This is the reason why we patch it to force it to await the flush. Otherwise only very few events would be sent to Sentry. It's not a great solution but better than nothing.
We're not sure about the response lifecycle here. Basically what we'd need to know for sure is if res.once('finish') is fired and awaited before res.end. @jiangbo0216 do you perhaps have any insights here?

[jiangbo0216]

Is "execution freezes" means flush won't call after res.end ?

[Lms24]

yes, that's what we're suspecting.

[jiangbo0216]

It seems that we haven't figured out why the "finish" event is not being emitted. It could be caused by different runtimes, so maybe we should consider using a different event name, such as "close."

How to reproduce this issue, we can try running the code and observing whether the "finish" event is being emitted properly.

Another solution could be to patch the "res.end" method, but without using any asynchronous operations. Instead, we can manually trigger the "finish" or "close" event when "res.end" is called.

res.end = function (chunk?: any | (() => void), encoding?: string | (() => void), cb?: () => void): void {
     this.emit('finish') // if is aws, emit manually
    _end.call(this, chunk, encoding, cb);
};

If have any ideas, let's work together to complete the PR.

Cannot merge this as-is, see my comment: #8849 (comment)

[Lms24]

I'll talk to the team on Monday, let's see how we can fix your original issue.

[jiangbo0216]

any update？

[Lms24]

No, sorry, this will have to wait until next week due to prioritization of other tasks.

[jiangbo0216]

No, sorry, this will have to wait until next week due to prioritization of other tasks.

any update? please

[lforst]

I don't think we should go forward with this PR. We need to patch and delay res.end for AWS lambdas. There is no way around it.

[mydea]

Hey, thanks again for opening this, in the current v8 branch we replaced all of this code with OpenTelemetry auto-instrumentation, so we are no longer manually patching these things ourselves. Still, thank you a lot for the contribution! 🙏