Skip to content
This repository has been archived by the owner on Feb 5, 2024. It is now read-only.

Ensure res is defined before calling res.resume() #113

Merged
merged 1 commit into from Dec 1, 2023

Conversation

alexmuller
Copy link
Contributor

My colleague Cynthia noticed this recent change to the plugin.

In production we are seeing multiple apps crashing because res is undefined.

This is a speculative fix which probably needs more work from someone who knows a lot more about this plugin than me.

We have ticket 1324529 open for this issue.

My colleague Cynthia noticed this recent change to the plugin.

In production we are seeing multiple apps crashing because res
is undefined.

This is a speculative fix which probably needs more work from
someone who knows a lot more about this plugin than me.

Co-Authored-By: Cynthia Mbulu <cyntibinti@users.noreply.github.com>
@alexmuller alexmuller requested a review from a team as a code owner December 1, 2023 12:53
@alexmuller
Copy link
Contributor Author

This was introduced recently in #112 - @thebanjomatic @colincasey is it possible that res can be undefined here?

We're seeing unhandled errors with the stack:

TypeError: Cannot read properties of undefined (reading 'resume')
    at ClientRequest.<anonymous> (/app/.heroku/heroku-nodejs-plugin/index.js:99:11)
    at ClientRequest.emit (node:events:517:28)
    at ClientRequest.emit (node:domain:489:12)
    at TLSSocket.socketErrorListener (node:_http_client:501:9)
    at TLSSocket.emit (node:events:517:28)
    at TLSSocket.emit (node:domain:489:12)
    at emitErrorNT (node:internal/streams/destroy:151:8)
    at emitErrorCloseNT (node:internal/streams/destroy:116:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

@apaleslimghost
Copy link

i think the correct fix here is to move the res.resume(); to after the err != null check. the callback is called with just the error from the res error handler which is why res is undefined here.

@colincasey
Copy link
Contributor

@apaleslimghost I agree, it would be better to move the logic to after err is checked but the code here will correctly prevent res.resume() from being called when res is undefined which is the main issue.

If I was planning on continuing maintenance on this plugin I would demand a bit more here but I'm currently trying to get this plugin retired and replaced with equivalent metrics using Node's Performance APIs instead of this C++ addon.

Thanks for reporting this @alexmuller. Apologies for not catching this when reviewing #112. This plugin has to be tested manually and I did not hit that edge case while verifying that the memory leak was fixed.

@colincasey colincasey enabled auto-merge (squash) December 1, 2023 15:32
@colincasey colincasey merged commit 154bf40 into heroku:main Dec 1, 2023
8 checks passed
colincasey added a commit to heroku/heroku-buildpack-nodejs that referenced this pull request Dec 1, 2023
Includes a fix where the response handler in the metrics plugin can cause a crash:
- heroku/heroku-nodejs-plugin#113
colincasey added a commit to heroku/heroku-buildpack-nodejs that referenced this pull request Dec 1, 2023
Includes a fix where the response handler in the metrics plugin can cause a crash:
- heroku/heroku-nodejs-plugin#113
@colincasey
Copy link
Contributor

@alexmuller this fix is included in the v231 release of the heroku/nodejs buildpack. Redeploying your application(s) should address the crashes you're experiencing.

@alexmuller
Copy link
Contributor Author

Thanks for fixing this so quickly @colincasey!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants