Clear wnb after close to prevent memory leak to Gateways. #5212
Conversation
There was a problem hiding this comment.
LGTM. Note, I would not consider this a memory leak, but if it is, then we should get to the bottom of it as to what is holding a reference to this connection? I know that we keep a rotation of closed connections, but that should not have a part into keeping a reference to this buffer. Also, this is not specific to gateways, just that in your setup this is affecting gateways because it goes over a poor link, but that would also apply to any other type of connections.
Lastly, are you using GOMEMLIMIT? If not, maybe that is why your server OOM'ed (again, assuming this is not a memory leak but just delayed GC).
It looks to me that the connection goes into reconnect when timeout errors are encountered. Is it supposed to close it entirely for a new one to open up? (when the destination is a gateway) |
|
Need to be especially careful with |
|
@davidzhao The @neilalexander As to the use of wnb without lock, you are right. However, flushAndClose() is only invoked from the writeLoop when it is detected that the connection is "closed" (marked as such) - unless the writeLoop was never started. So I think it should be safe. |
|
@derekcollison Let me experiment a bit more before we merge. I want to make sure that @neilalexander concerns are addressed. |
|
@davidzhao We should backtrack a bit. I had assumed that you found from profiling that the memory used was because of wnb, but is that the case? Since you claim that there are over 1M subscriptions, I wonder if the memory increase you see at each reconnect is not due to the fact that the server saves "closed connections" in a ring buffer, and that includes the list of subscriptions, and may be the reason for the memory usage. |
|
@davidzhao Hmm... never mind, after checking it looks like we save closed connections only for client and leaf connections, not other types. Digging more... |
There was a problem hiding this comment.
Ok, so I have verified that it is safe to access c.out.wnb from here because once a writeLoop is started, server code should always enqueue protocols for the writeLoop to send. The writeLoop is the one invoking flushOutbound and only if it has detected that the connection is closed will it call flushAndClose().
What I am not sure about is why not returning these buffers to the pool causes the memory growth. I mean, should not the GC reclaim those? I am not contesting that they should be returned to the pool, this is the right thing to do, but why is it not reclaimed if the connection is no longer referenced?
@neilalexander Is there anything particular about nbPoolGet that explains this?
@derekcollison I am approving this PR but would rather wait for @neilalexander to comment/approve.
|
GC will not reclaim if it does not think it needs to.. So in container based environments its critical to set GOMEMLIMIT to ~75% of the memory that is provisioned in the environment. If the memory provisioned is large, say 32GB, I suggest 16-20GB for GOMEMLIMIT for the server. |
|
@kozlovic Here's the profile from our production environment. This instance had high memory use and was close to getting killed.
I'm not seeing this path.. tracing the code reconnect would re-use the same object when connecting to remote gateways.
I could be wrong, but to me |
|
@kozlovic Yes, pools are emptied on a GC run, but if the GC doesn't think it needs to run yet, it's possible for them to build up in size. The problem may not be the pooled buffers though, it may be the length of the @davidzhao Can you please supply the full memory profile from |
@davidzhao, no, look at the reconnect code. In the case of an outbound GW connection, we end-up invoking this: Line 5530 in f1cd3ed Line 655 in f1cd3ed Line 672 in f1cd3ed Line 763 in f1cd3ed *client) is created here: Line 768 in f1cd3ed Starting at the first link I posted, there is no longer any reference to the original connection |
Thank you for keeping me honest. that's the thread that I missed. So the observed "leak" might just be that GC isn't reclaiming the memory. And I think the PR will still help to reduce allocations by returning these buffers back to the pool |
|
Update: we've set |
There was a problem hiding this comment.
Unfortunately, @neilalexander is right that this is "racy". I have seen a test fail with a data race when an internalSendLoop for system account is calling flushClients()->flushOutbound() which races with accessing it from here.
However, I have spent a lot of time to figure out what is holding a reference to the connection and I may have find it but need more time to investigate and see if that would prevent the memory usage without doing the cleaning here.
The main issue for router/gateway connections were that they would be registered with the global account but never removed from there, which would cause the connection to be retained. The memory leak was apparent in one user setup with large amount of subscriptions that were transfered to a slow gateway, causing accumulation of partial buffers before the connection was dropped. For gateway connections, we also needed to clean the outbound's map since it can contain sublist referencing its own connection. Same for subscriptions in c.subs. Another found cause for retained connection is with leafnode where we are using a subscription referencing the leafnode connection that was globally registered in a global sublist but never removed. Also the connection's c.out.sg (a `*Cond`) needed to be set to `nil` when the connection is closed. Without that, it seems that the connection would not be released (at least based on experimentation). We now make sure that c.out.sg is not nil before sending a signal. The bottom line is that it looks like having an object referencing itself in some way, prevents the GC from releasing the object, even if the "top level" object is no longer reachable. For instance, suppose `obj.someField = obj` seem to prevent the GC from releasing `obj`, even if `obj` itself is no longer referenced. The original issue/PR (#5212) was simply cleaning the c.out.wnb, but the way it was done was racy since c.out.wnb is used without lock in flushOutbound. Once the retain/release issue is fixed, cleaning this buffer is not really required (but good practice especially if not running with GOMEMLIMIT), so we take care of cleaning this up, but under the protection of the flushOutbound flag. If set, flushAndClose() will not do the cleaning, flushOutbound will do it. Relates to #5212 Signed-off-by: Ivan Kozlovic <[email protected]>
|
@davidzhao I have submitted PR #5244 to address the issue in a more systemic way. You should be referenced in the release notes should my PR be accepted. |
That's fantastic news! I'm looking forward to trying it out once it makes it into a release. In the mean time, we have been running on a patched build with clearing wnb and it has improved the memory footprint in our case. But you are right that it's addressing the symptom and not the root cause. |
…5244) The main issue for router/gateway connections were that they would be registered with the global account but never removed from there, which would cause the connection to be retained. The memory leak was apparent in one user setup with large amount of subscriptions that were transfered to a slow gateway, causing accumulation of partial buffers before the connection was dropped. For gateway connections, we also needed to clean the outbound's map since it can contain sublist referencing its own connection. Same for subscriptions in c.subs. Another found cause for retained connection is with leafnode where we are using a subscription referencing the leafnode connection that was globally registered in a global sublist but never removed. Also the connection's c.out.sg (a `*Cond`) needed to be set to `nil` when the connection is closed. Without that, it seems that the connection would not be released (at least based on experimentation). We now make sure that c.out.sg is not nil before sending a signal. The bottom line is that it looks like having an object referencing itself in some way, prevents the GC from releasing the object, even if the "top level" object is no longer reachable. For instance, suppose `obj.someField = obj` seem to prevent the GC from releasing `obj`, even if `obj` itself is no longer referenced. The original issue/PR (#5212) was simply cleaning the c.out.wnb, but the way it was done was racy since c.out.wnb is used without lock in flushOutbound. Once the retain/release issue is fixed, cleaning this buffer is not really required (but good practice especially if not running with GOMEMLIMIT), so we take care of cleaning this up, but under the protection of the flushOutbound flag. If set, flushAndClose() will not do the cleaning, flushOutbound will do it. Relates to #5212 Signed-off-by: Ivan Kozlovic <[email protected]>
|
@davidzhao Let's close this PR and the other has already been merged to main and should be available in nightly builds. If you have a chance to test it out, that would be great. |
The main issue for router/gateway connections were that they would be registered with the global account but never removed from there, which would cause the connection to be retained. The memory leak was apparent in one user setup with large amount of subscriptions that were transfered to a slow gateway, causing accumulation of partial buffers before the connection was dropped. For gateway connections, we also needed to clean the outbound's map since it can contain sublist referencing its own connection. Same for subscriptions in c.subs. Another found cause for retained connection is with leafnode where we are using a subscription referencing the leafnode connection that was globally registered in a global sublist but never removed. Also the connection's c.out.sg (a `*Cond`) needed to be set to `nil` when the connection is closed. Without that, it seems that the connection would not be released (at least based on experimentation). We now make sure that c.out.sg is not nil before sending a signal. The bottom line is that it looks like having an object referencing itself in some way, prevents the GC from releasing the object, even if the "top level" object is no longer reachable. For instance, suppose `obj.someField = obj` seem to prevent the GC from releasing `obj`, even if `obj` itself is no longer referenced. The original issue/PR (#5212) was simply cleaning the c.out.wnb, but the way it was done was racy since c.out.wnb is used without lock in flushOutbound. Once the retain/release issue is fixed, cleaning this buffer is not really required (but good practice especially if not running with GOMEMLIMIT), so we take care of cleaning this up, but under the protection of the flushOutbound flag. If set, flushAndClose() will not do the cleaning, flushOutbound will do it. Relates to #5212 Signed-off-by: Ivan Kozlovic <[email protected]>
The main issue for router/gateway connections were that they would be registered with the global account but never removed from there, which would cause the connection to be retained. The memory leak was apparent in one user setup with large amount of subscriptions that were transfered to a slow gateway, causing accumulation of partial buffers before the connection was dropped. For gateway connections, we also needed to clean the outbound's map since it can contain sublist referencing its own connection. Same for subscriptions in c.subs. Another found cause for retained connection is with leafnode where we are using a subscription referencing the leafnode connection that was globally registered in a global sublist but never removed. Also the connection's c.out.sg (a `*Cond`) needed to be set to `nil` when the connection is closed. Without that, it seems that the connection would not be released (at least based on experimentation). We now make sure that c.out.sg is not nil before sending a signal. The bottom line is that it looks like having an object referencing itself in some way, prevents the GC from releasing the object, even if the "top level" object is no longer reachable. For instance, suppose `obj.someField = obj` seem to prevent the GC from releasing `obj`, even if `obj` itself is no longer referenced. The original issue/PR (#5212) was simply cleaning the c.out.wnb, but the way it was done was racy since c.out.wnb is used without lock in flushOutbound. Once the retain/release issue is fixed, cleaning this buffer is not really required (but good practice especially if not running with GOMEMLIMIT), so we take care of cleaning this up, but under the protection of the flushOutbound flag. If set, flushAndClose() will not do the cleaning, flushOutbound will do it. Relates to #5212 Signed-off-by: Ivan Kozlovic <[email protected]>
The main issue for router/gateway connections were that they would be registered with the global account but never removed from there, which would cause the connection to be retained. The memory leak was apparent in one user setup with large amount of subscriptions that were transfered to a slow gateway, causing accumulation of partial buffers before the connection was dropped. For gateway connections, we also needed to clean the outbound's map since it can contain sublist referencing its own connection. Same for subscriptions in c.subs. Another found cause for retained connection is with leafnode where we are using a subscription referencing the leafnode connection that was globally registered in a global sublist but never removed. Also the connection's c.out.sg (a `*Cond`) needed to be set to `nil` when the connection is closed. Without that, it seems that the connection would not be released (at least based on experimentation). We now make sure that c.out.sg is not nil before sending a signal. The bottom line is that it looks like having an object referencing itself in some way, prevents the GC from releasing the object, even if the "top level" object is no longer reachable. For instance, suppose `obj.someField = obj` seem to prevent the GC from releasing `obj`, even if `obj` itself is no longer referenced. The original issue/PR (#5212) was simply cleaning the c.out.wnb, but the way it was done was racy since c.out.wnb is used without lock in flushOutbound. Once the retain/release issue is fixed, cleaning this buffer is not really required (but good practice especially if not running with GOMEMLIMIT), so we take care of cleaning this up, but under the protection of the flushOutbound flag. If set, flushAndClose() will not do the cleaning, flushOutbound will do it. Relates to #5212 Signed-off-by: Ivan Kozlovic <[email protected]>
The main issue for router/gateway connections were that they would be registered with the global account but never removed from there, which would cause the connection to be retained. The memory leak was apparent in one user setup with large amount of subscriptions that were transfered to a slow gateway, causing accumulation of partial buffers before the connection was dropped. For gateway connections, we also needed to clean the outbound's map since it can contain sublist referencing its own connection. Same for subscriptions in c.subs. Another found cause for retained connection is with leafnode where we are using a subscription referencing the leafnode connection that was globally registered in a global sublist but never removed. Also the connection's c.out.sg (a `*Cond`) needed to be set to `nil` when the connection is closed. Without that, it seems that the connection would not be released (at least based on experimentation). We now make sure that c.out.sg is not nil before sending a signal. The bottom line is that it looks like having an object referencing itself in some way, prevents the GC from releasing the object, even if the "top level" object is no longer reachable. For instance, suppose `obj.someField = obj` seem to prevent the GC from releasing `obj`, even if `obj` itself is no longer referenced. The original issue/PR (#5212) was simply cleaning the c.out.wnb, but the way it was done was racy since c.out.wnb is used without lock in flushOutbound. Once the retain/release issue is fixed, cleaning this buffer is not really required (but good practice especially if not running with GOMEMLIMIT), so we take care of cleaning this up, but under the protection of the flushOutbound flag. If set, flushAndClose() will not do the cleaning, flushOutbound will do it. Relates to #5212 Signed-off-by: Ivan Kozlovic <[email protected]>
We are seeing a memory leak whenever the gateway connections fail (and reconnects).
The sequence of events goes like this:
The problem is even after reconnection to gateways, the previous flush attempt buffers failed messages, including those that are too large and are timing out. When bandwidth limitation persists, it will lead to each reconnection attempt increasing memory usage by hundreds of MBs.
Signed-off-by: [email protected]