Putting objects into a Map, is always screaming to check for a potential memory leak. Maps will hold a strong reference to the Object, even though the Object is asssumed as destroyed. As the Map keeps a reference to the Object, it wil not get garbage collected.

Your close function is more a closeAll. You need to extend your eventHandler for session, so that the session listens on the close and other relevant events (error?, goaway?), and then you remove it from your sessionstore.
https://nodejs.org/api/http2.html#event-session
https://nodejs.org/api/http2.html#event-close

Well i am currently walking and did not test

So i did not test my assumption by instantiating a http2 server and connecting and disconnecting to the server, without closing the server itself and measure the memory.

But this should be considered.

Well, I modified your PR. Hope you are fine with that.

Thanks for your comments! It was my first pr and I'll take all your notes under advisement

@kostyak127
Hi,

just so that you dont feel bad, that I presumably "took over" your PR, I want to clarify, that I really appreciate your contribution.
When I was inverstigating your PR I of course checked out your PR and tested it locally and modified it. Thats why I had those suggestions in the comment I hid as obsolete. But 12 hours ago (midnight in Germany) I tested it further and realized it needs to be solved differently.

I dont know your skill level regarding Javascript. I want you to know that we encourage you and others to contribute to the project.

I personally dislike to remark problems when I have already the potential solution on my computer. It will result in a constant nit pick, about why the contributor did it not the way I expected it. So you would get the feeling that I nag you and you get potentially frustrated.

Yeah, that potential memory leak regarding Sets and Maps (or Arrays and Objects) is something, you learn usually, when your server crashes from OOM :D or others point it out to you, like in this PR.

If you want to learn more about this type of Memory leak, compare Map vs. WeakMap and Set vs. WeakSet.

In short:
Javascript has a dynamic memory management with garbage collection. You create a new Object, the engine allocates memory for it. But we dont have infinite memory, so the engine has to take care of it. It does that by a periodical garbage collection. But how does the engine know that? It keeps track of the references to an Object.

Example:

const a = []

function doSomething() {
  const b = {} 
}

If you know C++, then you read the curly braces differently. From StackOverflow Curly braces establish scope and lifetime. Objects created within a pair of curly braces are not accessible outside of them. They are not code; but, since the objects will likely be freed when you exit them, the compiler may produce code as a result of the closing curly brace.

Javascript is not that much different.

Variable b is instantiated in the function doSomething and inside those curly braces and thus inside that scope. While it is there, it will have a strong reference. When the function doSomething ended, the engine will remove the strong reference, making it possible to be garbage collected. Variable a on the other side, will not be gargabe collected, because it is existing in the outside scope.

If you modify the code as follows:

const a = []

function doSomething() {
  const b = {} 
  a.push(b)
}

Then b is assigned to a as an element. Via a strong(!) reference. So when doSomething is over, b is still marked via strong reference, and as such, will not be garbage collected.

Back to this PR:

You would not run into this issue, when you would have used a WeakMap/WeakSet. WeakMaps and WeakSets dont accept primitives but only Objects, because they are created especially to avoid MemoryLeaks. But from a WeakMap/WeakSet you can not retrieve the Objects you passed into it - they have no values() method-, because they keep a weak reference, thus they are named WeakSet and WeakMap. So thats why you need to use Map and Set with strong reference, so that you can call values()

Anyway as you see, now the code is adding listeners to the created Http2Session. When the session is connecting, it will add itself to that Set. And if it closes, it will remove itself from the Set. Thus ensuring, that the strong reference is not existing when the connection was closed, and thus the garbage collector can free the memory of the closed Session.

But there is still potential work for this PR. First of all, do we need to modify the documentation?

And furthermore, the more important question, is the implementation really complete? E.g. if a Http2Session times out, emitting a timeout event will it also result in a close event? Do we need then to remove it from the Set? Can a http2 client revive the old session, when it timed out? So we shouldnt remove it? Can there be other events, like error event be emmited?

Maybe it is necessary to read the code in nodejs sourcecode to figure this out.

@Uzlopak
Hi!

Thank you very much for the explanations! I truly appreciate that you took the time to point out and explain the issues in my code.

As for the current pull request, I would like to continue working on it and investigate when exactly sessions should be closed.

I’m not sure if this should be added to the documentation, because the current documentation (https://fastify.dev/docs/v5.2.x/Reference/HTTP2/) doesn’t describe configuration parameters (for example: http2SessionTimeout). I think I could use your advice on this.

I’m also not sure what the right approach is for this PR, since I’ll need some time to fully understand everything. Should I close it and continue the work in a new PR?

I think this PR is a good base to continue. It has a working solution for the general problem. If somebody wants to implement a solution for the corresponding issue, this person wouldnt need to start from scratch. Also we have an anti cookie licking policy.

Hi!

While I was researching how to properly handle session events, I looked into the Node.js source code as well as related issues to find problems similar to the one in Fastify — and here’s what I found:

A closed issue: nodejs/node#57611
And a PR that was successfully merged into Node.js — the fix is already available in v24.0.0.

I also verified that the issue does not reproduce on Node.js v24.0.0.

Maybe it makes sense to close this PR and the related issue (#5716)?

First of all: You did a good research. We know now, that node 24 does not have this issue. But we are supporting also node 20 and 22. So for those two we need fallback solutions. So we need to continue the work.

Hi!

I’ve done a bit of investigation into how certain events behave and also reviewed the Node.js documentation. Here’s what I found:

1. Session cleanup in Node.js (v24) source code

The Node.js source code removes the session on socket.on('close') and during session.destroy().
Since we can’t influence session.destroy(), I believe we should handle the events that are available to us explicitly.

2. List of Http2Session events

'close', 'connect', 'error', 'frameError', 'goaway', 'localSettings',
'ping', 'remoteSettings', 'stream', 'timeout'

'close'
Already handled — we correctly remove the session here.

'error'
• The documentation does not say that the session will be closed after an 'error' event.
• In Node.js source code, session.emit('error') is triggered during both session.destroy() and session.close().
• I tried to reproduce the 'error' event and observe session behavior when making a request after it emitted — but I couldn’t reproduce an 'error' without it being followed by a close() or destroy().
Proposal: Trust the documentation and do not assume that 'error' implies a closed session. So no need to delete the session on 'error'.

'frameError'
From the docs:

If the 'frameError' event is associated with a stream, the stream will be closed and destroyed.
If not associated with a stream, the Http2Session will be shut down.

Handler added:

session.once('frameError', function (type, code, streamId) {
  if (streamId === 0) {
    http2Sessions.delete(session)
  }
})

'goaway'
Docs say:

The Http2Session instance will be shut down automatically when the 'goaway' event is emitted.

However, I tested this and found that the session is only closed after the next request is received following a 'goaway'.

You can verify this behavior in my test repo:
https://github.com/kostyak127/http2-events-investigation

Proposal: Add a handler for 'goaway' and delete the session when it occurs.
I don’t think that it will impact performance, but it may help prevent potential memory leaks.

'timeout'
This is already handled inside Fastify.
The event listener calls session.close(), and we remove the session on 'close' accordingly.

Please let me know what you think about these suggestions.

P.S. i need to think a bit about how to cover the parts of code (Resolved)