-
Notifications
You must be signed in to change notification settings - Fork 0
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
Handle response data #21
Conversation
// throw away response | ||
resp.on('data',function() {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure response data is handled - otherwise Node.js won't end/finish up.
We don't need the response body (HTTP status code is enough) - so simply junk the data received.
I assume the Node.js v20 stream implementation leaves behind a latent Promise/etc. in the case the data returned isn't consumed (maybe?). Couldn't find a concrete change/reason in online docs/etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a known bug? Seems like a pretty basic requirement to fire-and-forget requests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so @jack-green - as we're a few point releases into v20
now and assume it's "desired". But without any code changes, moving between v18
-> v20
- the HTTPS request is made (and Slack message posted) - but the Node process will then "hang" indefinitely - so I can only assume it's a latent Promise awaiting the resolve?
In GitHub Actions land - the action is forcefully killed after three minutes - due to this inactivity - which is nice - but isn't a solution :)
Found this - but this goes right back to Node.js 0.10
- but seems to hint it's a paused socket that remains - this could make sense too.
const https = require('node:https'), | ||
url = require('node:url'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use node:*
naming for stdlib requires.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're now the tip of the node spear, we're still stuck on 18.12 in yewie land at the moment!
After the bump to Node.js
v20
with #20 - it's become clear that ahttps.request()
must havedata
events handled within the response - otherwise Node.js will hang/wait and will not exit.This resulted in a Slack message that would be sent instantly as per before - but then the workflow job would "hang" until tje GitHub Actions runner finally times the process out after three minutes of inactivity. Ouch!
So this change will add a dummy handler of
resp.on('data',function() {});
and throw away the response payload (we don't need/want it).Also:
@actions/core
and@actions/github
.esbuild
.require()
s for stdlib modules withnode:*
.