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
fix(NODE-5127): implement reject kmsRequest on server close #3964
Conversation
…request-onclose
.once('timeout', () => rejectOnNetSocketError(ontimeout())) | ||
.once('error', err => rejectOnNetSocketError(onerror(err))) | ||
.once('close', () => rejectOnNetSocketError(onclose())) | ||
.once('connect', () => resolveOnNetSocketConnect()); |
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.
[nit/question] Is there any reason, apart from style consistency, not to do this?
.once('connect', () => resolveOnNetSocketConnect()); | |
.once('connect', resolveOnNetSocketConnect); |
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.
Here, no there isn't but I also would not change it.
Generally, you may want to be careful about passing a function directly into another API. Adding parameters to the listener would be considered a feature and not a breaking change, but our promise would resolve with whatever new parameters were added.
Description
Handle
onclose
event in kmsRequest.What is changing?
onclose
handler to the socketIs there new documentation needed for these changes?
None
What is the motivation for this change?
NODE-5127
NODE-3959
Release Highlight
Fixed unresolved request issue in KMS requester
Internal to the field-level encryption machinery is a helper that opens a TLS socket to the KMS provider endpoint and submits a KMS request. The code neglected to add a
'close'
event listener to the socket, which had the potential to improperly leave the promise pending indefinitely if no error was encountered.Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript