TrueCloudLab/frostfs-sdk-go
33
0
Fork 21
Code Issues 34 Pull requests 1 Releases Activity Actions

Fix connection leak and panic at close operation #171

Merged
alexvanin merged 3 commits from alexvanin/frostfs-sdk-go:fix/pool-panic-leak into master 2024-09-04 19:51:15 +00:00
Conversation 0 Commits 3 Files changed 2 +5 -1
1 changed files with 5 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 8a04638749 - Show all commits

4
pool/pool.go
View file

@ -86,6 +86,8 @@ type client interface {
type clientStatus interface { type clientStatus interface {
// isHealthy checks if the connection can handle requests. // isHealthy checks if the connection can handle requests.
isHealthy() bool isHealthy() bool
// isDialed checks if the connection was created.
dstepanov-yadro commented 2023-10-03 16:06:15 +00:00
Outdated
Review
Copy link

isDialled -> isDialed

isDialled -> isDialed
👍 1
isDialed() bool
// setUnhealthy marks client as unhealthy. // setUnhealthy marks client as unhealthy.
setUnhealthy() setUnhealthy()
// address return address of endpoint. // address return address of endpoint.
@ -2809,9 +2811,11 @@ func (p *Pool) Close() {
// close all clients // close all clients
for _, pools := range p.innerPools { for _, pools := range p.innerPools {
for _, cli := range pools.clients { for _, cli := range pools.clients {
if cli.isDialed() {
fyrchik commented 2023-10-03 14:13:56 +00:00
Outdated
Review
Copy link

Wouldn't it be better from the client POV to just allow closing even undialed clients?

Wouldn't it be better from the client POV to just allow closing even undialed clients?
alexvanin commented 2023-10-03 16:46:28 +00:00
Outdated
Review
Copy link

It would, but simple solution down below didn't work, so I suppose changes should be done on api-go level as well.

func (c *Client) Close() error {
	if c.c.Conn() != nil {
		return c.c.Conn().Close()
	}
	return nil

Function comment clearly tells us MUST NOT be called before successful Dial. So invoker (pool) should check dial state, which it does now.

It would, but simple solution down below didn't work, so I suppose changes should be done on api-go level as well. ```go func (c *Client) Close() error { if c.c.Conn() != nil { return c.c.Conn().Close() } return nil ``` Function comment clearly tells us `MUST NOT be called before successful Dial`. So invoker (pool) should check dial state, which it does now.
_ = cli.close() _ = cli.close()
} }
} }
}
} }
// SyncContainerWithNetwork applies network configuration received via // SyncContainerWithNetwork applies network configuration received via