Fix memory leak on persistent connections

[willcosgrove]

I noticed a memory leak in my Rails app. The app uses the searchkick gem for searching using elasticsearch. That in turn uses the faraday gem. I had opted to use httpx via it's faraday adapter so I could take advantage of a persistent connection to the elasticsearch server.

But recently I started noticing a memory leak in my application. After doing some investigative work on some heap dumps, I started to zero in on httpx, http-2-next, and now after a recent update, http-2.

It is at this point that I should stress: I have no idea what I'm doing. I've made a change here. It makes sense to me, as someone who knows literally nothing about what is required to build or design a working http/2 client. But it seemed like it could be the source of my leak, so I decided to patch it and run it in production on a Friday. 🎉

I'm now about 4 hours in to my inadvisable experiment, and astonishingly nothing seems to have broken. And the memory profile is now flat again!

I have no notion that this PR is worth merging in as is. But I wanted to open it just to say, that I'm fairly confident that there exists a memory leak in the HTTP2::Connection class as it is right now. And I'm reasonably confident that this change (however incorrect it is in other ways) seems to fix it, and not immediately cause other problems.

Thanks for your work on this gem. I hope this PR helps in some way, even if it isn't by providing mergable code.

[HoneyryderChuck]

Hi @willcosgrove ,

Thx for the patch. Indeed, it was a regression from this commit. Until then, stream was being removed only after the grace period. Still, I think your patch has legs, as the code already seems to handle frame selection on recently closed frames, while erroring on the rest. I'll do a bit more consideration til next week.

[HoneyryderChuck]

fixed the failing tests, which were exposing incorrect behaviour of leaving streams hanging around in @streams. Managed to find an issue with stream order verification, as well as dealing with RST_STREAM on already closed streams.

@igrigorik @alextwoods can I ask you for a review on the changes here, given that some of them are mine? This seems an improvement on the state of stream close lifecycle management found on 0.12 (which only removed them on recently closed streams cleanup).

[mullermp]

LGTM

[willcosgrove]

Just chiming in here to say that I updated our production deployment to 8f072b0 and it continues to run smoothly with no more memory leak

[HoneyryderChuck]

v1.0.1 released.