Fix memory leak during signing #596
Conversation
| if (binding->node_external != NULL) { | ||
| napi_delete_reference(env, binding->node_external); | ||
| } |
There was a problem hiding this comment.
This fixes another leak I found when searching for uses of napi_create_reference()/napi_delete_reference())
No one is using the NodeJS bindings of our HTTP classes, as far as I know...
| if (binding->node_external != NULL) { | ||
| napi_delete_reference(env, binding->node_external); | ||
| } |
There was a problem hiding this comment.
This fixes another leak I found when searching for uses of napi_create_reference()/napi_delete_reference())
No one is using the NodeJS bindings of our HTTP classes, as far as I know...
| @@ -115,20 +115,6 @@ struct aws_http_message *aws_napi_http_message_unwrap(napi_env env, napi_value j | |||
| * Constructor | |||
| **********************************************************************************************************************/ | |||
|
|
|||
| static void s_napi_http_request_finalize(napi_env env, void *finalize_data, void *finalize_hint) { | |||
There was a problem hiding this comment.
There were 2 different finalizers for the same class. I combined them into 1.
(When this code was first written, the 2 different finalizers were necessary because the underlying C classes didn't have reference-counting. But now they do, so the weirdness the led to needing 2 different finalizers isn't necessary anymore)
| AWS_NAPI_ENSURE(env, napi_create_reference(env, result, 1, &binding->node_headers)); | ||
| } | ||
|
|
||
| /* Store the value for later */ | ||
| AWS_NAPI_ENSURE(env, napi_create_reference(env, result, 1, &binding->node_headers)); | ||
|
|
||
| return result; |
There was a problem hiding this comment.
This was the leak. We were creating a new napi_ref each time the headers were queried. Now, we only create a napi_ref if we didn't already have one.
| @@ -74,7 +74,7 @@ static void s_on_response_call(napi_env env, napi_value on_response, void *conte | |||
| } | |||
|
|
|||
| /* clean up the response buffer */ | |||
| aws_http_message_destroy(binding->response); | |||
| aws_http_message_release(binding->response); | |||
There was a problem hiding this comment.
this isn't a bugfix, just replacing a deprecated _destroy() function with a modern _release() function
Issue: (internal ticket V1637484435)
When doing SigV4A signing from aws-sdk-js-v3, memory usage gradually increases due to memory leaks.
Description of changes:
Fix leak that occurred if headers were queried repeatedly.
Future Work:
All native unit tests should check that they aren't leaking memory. I spent a few hours trying to add such tests, but couldn't get it working in a reliable way. I didn't want this to hold back the fix, so leaving it for another day. See these blog posts for hints on various techniques NodeJS has used for leak testing through the years: https://joyeecheung.github.io/blog/2024/03/17/memory-leak-testing-v8-node-js-1/, https://joyeecheung.github.io/blog/2024/03/17/memory-leak-testing-v8-node-js-2/
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.