Possible memory leak in Bangle.js gadgetbridge?

[espruino-discuss2]

Posted at 2023-05-25 by user155558

Hello,

I am calling regularly Bangle.http("https://...") from my Bangle.js 2. For testing, I do it at a rate of 1 request/sec, while ensuring not starting a new request before the previous one returns.

After establishing the Bluetooth connection to gadgetbridge, it works fine for some time, but after some minutes, issues occur. These are:

• random bluetooth disconnects
• unresponsive hanging of Gadgetbridge
• bluetooth reconnect feature not working reliably

I have enabled USB-Debugging and tried to track the memory usage of gadgetbridge:

watch -n1 adb shell dumpsys meminfo com.espruino.gadgetbridge.banglejs

and I see that the "TOTAL" memory is increasing constantly at a rate of about 1MB/s.

Is it possible that there is a memory leak in Bangle.js gadgetbridge and has somebody else seen this behaviour already? How can I track this down further?

I would appreciate greatly any help.

Many thanks!

[espruino-discuss2]

Posted at 2023-05-26 by @gfwilliams

I see that the "TOTAL" memory is increasing constantly at a rate of about 1MB/s.

Oh wow, that's huge.

It is possible there's a memory leak, yes. I keep an eye on Bangle.js itself but a leak on the Android side is not something I've tested for myself.

I'm not an Android developer so I'm not sure I can really be more help than Google about how to track it down... I think you'd have to do your own build, but the code responsible for the HTTP request is https://codeberg.org/Freeyourgadget/Gadgetbridge/src/branch/master/app/src/main/java/nodomain/freeyourgadget/gadgetbridge/service/devices/banglejs/BangleJSDeviceSupport.java#L591-L701

I wonder if it's related to Volley.newRequestQueue - maybe that should just be called once?

I'm afraid I'm on holiday next week so I'm not going to be able to help tracking this down until my return though

---

Posted at 2023-05-26 by rigrig

I wonder if it's related to Volley.newRequestQueue - maybe that should just be called once?

I think that's it: looking at this, we should either call stop() on the queue once we have a response, or set up a singleton. Otherwise every request leaves a new "thread pool" lying about.

If you just need to make a one-time request and don’t want to leave
the thread pool around, you can create the RequestQueue wherever you
need it and call stop() on the RequestQueue once your response or
error has come back, using the Volley.newRequestQueue() method
described in Sending a Simple Request. But the more common use case is
to create the RequestQueue as a singleton to keep it running for the
lifetime of your app, as described in the next section.

(Calling stop() seems by far the easiest solution.)

---

Posted at 2023-05-26 by user155558

Hello Gordon, hello rigrig,

many thanks for replying to this so quickly, it's very nice from you. In fact, I also have only very limited time these days, but I want to try what you suggested as soon as I can.

---

Posted at 2023-05-30 by user155558

I made a test and ensuring the queue.stop(); queue=null; before creating a new request queue resolved the memory leak.

Many thanks again for finding this.

My test was quite dirty however, I think the release of memory should be done in the response and error handler functions to allow for interleaved requests. I'm not confident enough with android development, so maybe the android developer could add this and commit it to one of the upcoming releases?

---

Posted at 2023-05-30 by @thyttan

If you're up for it I'd suggest that you make a pull request to the gadgetbridge repo. The more of us that know how to add/tweak functionality there the better in my opinion.

I've done a couple myself with no previous knowledge before that - I'd be happy to try and help with any question you'd have. And the maintainers of gadgetbridge are helpful when they have the time.

---

Posted at 2023-05-31 by rigrig

Actually, we might get away with just making it an attribute, and only calling Volley.newRequestQueue for the very first request (and cleaning it up in dispose). That way there is no need to sprinkle stop over all response/error handler code, and we don't need a full-blown singleton class either.

Looking at 400-line switch statements makes me itchy though, so I'll try to submit a PR with some refactoring Soon, and include a fix for this.

edit: PR

---

Posted at 2023-06-05 by @gfwilliams

Great - thanks for that! That fix looks perfect

---

Posted at 2023-06-10 by user155558

Many thanks for making the pull request, I'm very happy with it!