fix: avoid memory leaks due to undisposed promise resources#885
fix: avoid memory leaks due to undisposed promise resources#885kjin merged 6 commits intogoogleapis:masterfrom
Conversation
ghost
assigned
googlebot
added
the
cla: yes
ofrobots
left a comment
ofrobots
left a comment
There was a problem hiding this comment.
Is it possible to add a test for leaking promise resources?
| // currently running. | ||
| this.contexts[id] = this.contexts[this.ah.executionAsyncId()]; | ||
| } else { | ||
| } else if (this.contexts[triggerId] !== undefined) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@ofrobots I have added tests. They fail when the source changes are not applied. |
| if (type === 'PROMISE') { | ||
| if (type === 'PROMISE' && this.contexts[currentId] !== undefined) { | ||
| // Opt not to use the trigger ID for Promises, as this causes context | ||
| // confusion in applications using async/await. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| delete this.contexts[id]; | ||
| }, | ||
| promiseResolve: (id: number) => { | ||
| // Some Promise async resources do not get destroyed with a destroy |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| // Create the hook. | ||
| this.hook = this.ah.createHook({ | ||
| init: (id: number, type: string, triggerId: number, resource: {}) => { | ||
| const currentId = this.ah.executionAsyncId(); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
ofrobots
left a comment
ofrobots
left a comment
There was a problem hiding this comment.
lgtm once you make the suggested structural improvements.
4 participants
Most Promise async resources require a separate
promiseResolveevent listener. See the inline comments for more details.