Skip to content
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: memory leak with when using Http(s)Agent keepAlive=true #391

Merged
merged 7 commits into from
Dec 15, 2023

Conversation

notanengineercom
Copy link
Contributor

@notanengineercom notanengineercom commented Nov 28, 2023

Fixes #390
This pull request contains:

The spec fails without the fix: when keep-alive is enabled, the tcp socket events register twice (once per API call):

[test] 1) integration HTTP event callbacks keep alive
[test] Message:
[test] Expected 2 to be 1.
[test] Stack:
[test] at
[test] at Object.verifySockets (/Users/notanengineer/Documents/GitHub/mappersmith/spec/integration/node/support/keep-alive.js:13:46)
[test] at /Users/notanengineer/Documents/GitHub/mappersmith/spec/integration/node/integration.spec.js:95:27
[test] at processTicksAndRejections (node:internal/process/task_queues:96:5)
[test] Message:
[test] Expected 2 to be 1.
[test] Stack:
[test] at
[test] at Object.verifySockets (/Users/notanengineer/Documents/GitHub/mappersmith/spec/integration/node/support/keep-alive.js:14:47)
[test] at /Users/notanengineer/Documents/GitHub/mappersmith/spec/integration/node/integration.spec.js:95:27
[test] at processTicksAndRejections (node:internal/process/task_queues:96:5)
[test]
[test] 20 specs, 1 failure
[test] Finished in 0.819 seconds

Copy link

changeset-bot bot commented Nov 28, 2023

🦋 Changeset detected

Latest commit: d3ef379

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mappersmith Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@allanpaiste
Copy link

@klippx @tulios perhaps you could find time to confirm the validity of this PR and get this merged/released? :)

Copy link
Collaborator

@klippx klippx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, really great, I appreciate the effort! I hope you don't mind that I have some humble comments from my perspective, please give me your feedback and we can agree what makes sense and what does not. 🙏

src/gateway/http.ts Show resolved Hide resolved
src/gateway/http.ts Outdated Show resolved Hide resolved
spec/integration/node/support/keep-alive.js Outdated Show resolved Hide resolved
spec/integration/node/integration.spec.js Outdated Show resolved Hide resolved
spec/integration/node/integration.spec.js Outdated Show resolved Hide resolved
spec/integration/node/integration.spec.js Show resolved Hide resolved
spec/integration/node/support/keep-alive.js Outdated Show resolved Hide resolved
@notanengineercom
Copy link
Contributor Author

@klippx thanks for the great feedback and review, I really liked the suggestions. The only pending task is the test for the onRequestSocketAssigned callback I believe. Let me know if you have any other suggestions!
One thing I learnt: to not rush code after work 🙈

@klippx
Copy link
Collaborator

klippx commented Dec 14, 2023

One thing I learnt: to not rush code after work 🙈

Don't worry, I appreciate that you got a PR up including test case and all, that's more than most people will put up!

Pending tasks

  • Add test for the onRequestSocketAssigned callback

@notanengineercom
Copy link
Contributor Author

notanengineercom commented Dec 14, 2023

Pending tasks

  • Add test for the onRequestSocketAssigned callback

I didn't mention it in a comment, but yesterday I pushed a commit with the test for the onRequestSocketAssigned callback 😄

@klippx klippx merged commit 9da82f6 into tulios:master Dec 15, 2023
7 checks passed
@notanengineercom notanengineercom deleted the fix/keep-alive branch December 15, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: memory leak with when using Http(s)Agent keepAlive=true
3 participants