fix: fix memory leak in reaper connect function #3201
Conversation
…ly to avoid to memory leak Signed-off-by: Sagar Vakkala <[email protected]>
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
ionicc
changed the title
ionicc
changed the title
| @@ -1335,15 +1335,14 @@ func (p *DockerProvider) ReuseOrCreateContainer(ctx context.Context, req Contain | |||
|
|
|||
| termSignal, err := r.Connect() | |||
| if err != nil { | |||
| if termSignal != nil { | |||
There was a problem hiding this comment.
question: I don't see a flow where r.Connect can return an error and termSignal, it appears to be one or the other. Can you clarify where this would come from?
There was a problem hiding this comment.
@stevenh Hmm, From my observation of term signal handling in these methods, The defer func written to pass the term signal is redundant as the method returns with an error before the defer func is set. See below:
From: https://github.com/testcontainers/testcontainers-go/blob/main/docker.go#L1336-L1347
termSignal, err := r.Connect()
if err != nil {
return nil, fmt.Errorf("reaper connect: %w", err) <-- Returns
}
// Cleanup on error.
defer func() { <-- Never gets set in the case of error present
if err != nil {
termSignal <- true
}
}()Due to this, There's a possibility of a memory leak in the case both, a term signal and an error is passed. This is also the conclusive point of the issue linked.
There was a problem hiding this comment.
That's not the case as err and termHandker can't both be set and err is the captured err so any error which is returned from then on will result in the term signal being handled before return.
What evidence of memory leak do you have?
There was a problem hiding this comment.
@ionicc any thoughts on the above?
There was a problem hiding this comment.
@stevenh Sorry for my late reply on this, I didn't get a chance to dig deeper into the mutually exclusive nature of r.Connect() for termSignal and err. I understand your point, the shadowing of the function signature err by original errors that are being instantiated through the func will not apply to the captured error case, Is that the intention? For ex in https://github.com/testcontainers/testcontainers-go/blob/main/docker.go#L1371, The function signature will be nil and would not be caught by the defer func.
There was a problem hiding this comment.
That's actually not shadowed, new vars are instantiated left to right only if needed, so where err is already defined a new one is not created.
In a defer statement where a named variable is in play, the value is always that of return value not what was last assigned to that variable. You can see this behaviour here.
So what the defer is doing is ensuring that the termSignal is triggered if the function encounters an error before returning.
What does this PR do?
Adds a fix to pass termination signal to reaper in the context of an error. The previous implementation had a memory leak issue due to the usage of a redundant check on an error which was returned before reaching the statement of passing the termination signal.
Why is it important?
This will make sure a memory leak does not happen if reaper.connect returns an error and properly sends a termination signal.
Related issues