Avoid connection leak in tree pool with netmap support #331

Merged
alexvanin merged 3 commits from alexvanin/frostfs-sdk-go:fix/tree-pool-connection-leak into master 2025-02-05 07:47:25 +00:00
Owner

To avoid connection leak, call close() immediately after failed connection is established. In regular tree pool, unhealthy connections are handled by background goroutine which calls redialIfNecessary() to reestablish connection. Here it is not viable so connection must be close.

Kudos to @dstepanov-yadro for pointing out this code.

To avoid connection leak, call `close()` immediately after failed connection is established. In regular tree pool, unhealthy connections are handled by background goroutine which calls `redialIfNecessary()` to reestablish connection. Here it is not viable so connection must be close. Kudos to @dstepanov-yadro for pointing out this code.
alexvanin added the
bug
label 2025-02-04 18:14:49 +00:00
alexvanin self-assigned this 2025-02-04 18:14:49 +00:00
alexvanin added 2 commits 2025-02-04 18:14:49 +00:00
To avoid connection leak, call `close()` immediately after connection
is established. In regular tree pool, unhealthy connections are handled
by background goroutine which calls `redialIfNecessary()` to reestablish
connection. Here it is not viable so connection must be close.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
[#xxx] pool: Add mocked test tree service and check goroutine leak
Some checks failed
Code generation / Generate proto (pull_request) Successful in 34s
DCO / DCO (pull_request) Failing after 33s
Tests and linters / Tests (pull_request) Successful in 45s
Tests and linters / Lint (pull_request) Successful in 1m37s
70fbc5ae27
Use real gRPC connection in new mocked tree service.

Signed-off-by: Alex Vanin <a.vanin@yadro.com>
alexvanin force-pushed fix/tree-pool-connection-leak from 70fbc5ae27 to 4ecbfb0edf 2025-02-04 18:15:24 +00:00 Compare
alexvanin reviewed 2025-02-04 18:18:27 +00:00
@ -0,0 +227,4 @@
routinesBefore := runtime.NumGoroutine()
for i := 0; i < 1000; i++ {
_, err = treePool.AddNode(context.Background(), AddNodeParams{CID: cnr})
require.NoError(t, err)
Author
Owner

I will appreciate any other way to check the leak. I had no better idea than checking number of routines directly. gRPC server does not provide any useful data about connections as far as I understand.

I will appreciate any other way to check the leak. I had no better idea than checking number of routines directly. gRPC server does not provide any useful data about connections as far as I understand.
Member

https://github.com/uber-go/goleak

But I don't think we are going to introduce a new import. I'd use the same idea like you with runtime :)

https://github.com/uber-go/goleak But I don't think we are going to introduce a new import. I'd use the same idea like you with `runtime` :)
alexvanin changed title from WIP: Avoid connection leak in tree pool with netmap support to Avoid connection leak in tree pool with netmap support 2025-02-04 18:19:56 +00:00
requested reviews from storage-services-developers, storage-core-committers, storage-core-developers, storage-services-committers 2025-02-04 18:19:56 +00:00
alexvanin reviewed 2025-02-04 18:29:18 +00:00
@ -0,0 +42,4 @@
ni := apinetmap.NodeInfo{}
ni.SetAddresses(server.lis.Addr().String())
ni.SetPublicKey(server.key.PublicKey().Bytes())
err := nodes[i].ReadFromV2(ni) // no other way to set address field in netmap.NodeInfo
Author
Owner

Let me know if there any

Let me know if there any
aarifullin approved these changes 2025-02-04 20:53:14 +00:00
Dismissed
aarifullin left a comment
Member

👍

👍
dkirillov approved these changes 2025-02-05 06:31:52 +00:00
Dismissed
dstepanov-yadro approved these changes 2025-02-05 06:31:58 +00:00
Dismissed
acid-ant approved these changes 2025-02-05 07:20:52 +00:00
Dismissed
alexvanin added 1 commit 2025-02-05 07:44:09 +00:00
[#331] pool: Fix 'sortServers' in tree pool server test
All checks were successful
DCO / DCO (pull_request) Successful in 30s
Code generation / Generate proto (pull_request) Successful in 36s
Tests and linters / Tests (pull_request) Successful in 47s
Tests and linters / Lint (pull_request) Successful in 1m37s
5a35fa4353
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
alexvanin dismissed aarifullin's review 2025-02-05 07:44:09 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

alexvanin dismissed dkirillov's review 2025-02-05 07:44:09 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

alexvanin dismissed dstepanov-yadro's review 2025-02-05 07:44:09 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

alexvanin dismissed acid-ant's review 2025-02-05 07:44:09 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

Author
Owner

Fixed for loop in sortServers in the test.

Fixed `for` loop in `sortServers` in the test.
dkirillov approved these changes 2025-02-05 07:46:49 +00:00
Author
Owner

Merging based on previously approved review.

Merging based on previously approved review.
alexvanin merged commit 5a35fa4353 into master 2025-02-05 07:47:25 +00:00
alexvanin deleted branch fix/tree-pool-connection-leak 2025-02-05 07:47:28 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-sdk-go#331
No description provided.