Fix memory leak in subscription "message-id to topic" mapping #1535
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bverhoeven
force-pushed
the
message-id-to-topic-leak
branch
from
df7b8d1
to
477857b
Compare
robertsLando
approved these changes
Nov 14, 2022
|
@YoDaMa ping |
|
@vishnureddy17 @ryanwinter @BertKleewein anyone here that can kindly merge this PR? |
vishnureddy17
approved these changes
Jun 22, 2023
robertsLando
added a commit
that referenced
this pull request
Jul 3, 2023
…1622) @vishnureddy17 Fix typo in README 00bf657 @bverhoeven Fix memory leak in subscription topic mapping (#1535) 8c77eec @mwohlert @robertsLando @ynagasaki feat: allow user to disable pre-generated write cache (#1151) 0d11888 Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com> Co-authored-by: Yoshi Nagasaki <yn253@cornell.edu> Co-authored-by: Dmitry Kurmanov <kurmanov.work@gmail.com> Co-authored-by: Vishnu Reddy <vishnureddy@microsoft.com> Co-authored-by: Bas Verhoeven <soczol@gmail.com> Co-authored-by: Michel Wohlert <michel.wohlert@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We've been using this library extensively in a project that makes a lot of subscriptions and have noticed that it's been leaking memory (which is causing NodeJS to crash with "JavaScript heap out of memory").
We've pinpointed this to the MQTT.js library's "message id to subscription" mapping:
A message ID is assigned and stored in
messageIdToTopicwhenever a new subscription is started and resubscription is enabled (which is the default). The message-id to topic mapping is never cleared and keeps accumulating over time, which creates a memory leak that can not be resolved by the caller.This mapping seems to have been introduced as a way to prevent resubscriptions of topics that have failed to subscribe on their first attempt, as introduced in as part of #665 in f6d12f0
There's, unless I'm overlooking something, no reason to keep this mapping in place once an acknowledgement is received. With that assumption, I've created a few tests to reproduce the issue and have created a simple fix that clears the mapping when
SUBACKis received.We've been running a patched version of the library and have noticed a significant reduce in memory usage and can confirm that, for us, the memory leak has been resolved. Would love to get this included upstream though. If there's something that we missed, I'd love to know too!