search

clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

Fix buffer leak in `test-idle-timeout` #722

Closed DerGuteMoritz closed 2 months ago

DerGuteMoritz commented 3 months ago

The client idle-timeout tests were using raw handlers but the local echo-handler didn't release the body buffers. Fix by not using raw handlers in the first place (which was unnecessary here).

Took the liberty to also enable leak detection for the whole test suite in CI with this PR.

Closes #707.

DerGuteMoritz commented 2 months ago

It might be worth checking the history to see why they were testing this with raw handlers. Was it just to be thorough, or was there more to it?

Good call! @arnaudgeiser looks like you introduced the test with the :raw-stream? true option in place with 67ceb66a2135de5c5cf5600e0e82008c8525b903 - do you recall any particular motivation?

That being said, we could also go all-in and convert the whole test to use with-both-handlers instead!

arnaudgeiser commented 2 months ago

Good call! @arnaudgeiser looks like you introduced the test with the :raw-stream? true option in place with https://github.com/clj-commons/aleph/commit/67ceb66a2135de5c5cf5600e0e82008c8525b903 - do you recall any particular motivation?

It was just to be sure it was working the idle-handler was working in both scenarios. But at that time, it was absolutely not related to buffers leaking.

DerGuteMoritz commented 2 months ago

That being said, we could also go all-in and convert the whole test to use with-both-handlers instead!

I briefly looked into this but with-both-handlers doesn't allow passing in extra server options, so we can't just switch to it. However, when testing it with raw handler again, I noticed that it still failed even with the handler supposedly fixed (i.e. releasing all body buffers). Turns out that there was actually still a leak hiding in the HTTP/2 server implementation, namely when skipping over empty buffers! See the new commit I just pushed for the fix.

KingMob commented 2 months ago

Wow, that's a massive (if easily-fixed) bug. I'm surprised we hadn't already noticed it. It should have been dropping all data frames sent by a client.

DerGuteMoritz commented 2 months ago

It should have been dropping all data frames sent by a client.

How so? Only empty ones were dropped - note that I changed the when to when-not there. The only bug really was that we leaked those empty buffers in raw mode because the handler never got to see them while the rest of the server code went on to assume that the handler takes care of releasing them. Or am I missing something? :thinking:

KingMob commented 2 months ago

Ahh, nm. I hadn't noticed you changed the test from > to == when refactoring out content-empty? It looked like it had been doing the wrong thing.

DerGuteMoritz commented 2 months ago

Ah right, yeah, found it easier to name this way (I guess have-content? could have worked :smile:). Alright, glad we cleared that up, merging away!