Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Shutdown TimeoutManager during node Shutdown #1707

Merged
merged 102 commits into from
Oct 23, 2023
Merged

Shutdown TimeoutManager during node Shutdown #1707

merged 102 commits into from
Oct 23, 2023

Conversation

abi87
Copy link
Contributor

@abi87 abi87 commented Jul 7, 2023 •
edited

Why this should be merged

@darioush introduced goleak to fix some goroutines leak in coreth UTs. I applied it to some avalanchego packages and came up with this cleanup. I believe we are leaking some goroutines in production code too.

How this works

Just add a main_test.go in a few package and run UTs as usual. An error log comes up if goroutines leak. I fix them, mostly just duly called a shutdown method wherever needed.

How this was tested

Fixes mainly UTs + timeoutManager in prod code

@abi87 abi87 requested a review from darioush July 7, 2023 16:01
darioush
darioush reviewed Jul 7, 2023
View reviewed changes
go.mod Outdated Show resolved Hide resolved
abi87
abi87 commented Jul 7, 2023
View reviewed changes
vms/platformvm/service_test.go Outdated
Comment on lines 727 to 730
defer func() {
require.NoError(service.vm.Shutdown(context.Background()))
service.vm.ctx.Lock.Unlock()
}()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoids leaking timer goroutine, similarly to other tests

abi87
abi87 commented Jul 7, 2023
View reviewed changes
vms/platformvm/vm_test.go
@@ -1561,6 +1561,7 @@ func TestBootstrapPartiallyAccepted(t *testing.T) {
require.NoError(err)

go timeoutManager.Dispatch()
defer timeoutManager.Stop()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoids leaking timer goroutine

@abi87 abi87 changed the title fixed some UTs goroutine leak fixed some goroutine leak in UTs and prod code Jul 7, 2023
@abi87 abi87 requested a review from darioush July 7, 2023 20:02
abi87
abi87 commented Jul 7, 2023
View reviewed changes
node/node.go
@@ -125,6 +125,9 @@ type Node struct {
// Build and parse messages, for both network layer and chain manager
msgCreator message.Creator

// Manages network timeouts
timeoutManager timeout.Manager
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making this a node attribute to duly stop it upon shutdown.

@abi87 abi87 self-assigned this Jul 7, 2023
@abi87 abi87 requested a review from dhrubabasu July 7, 2023 22:49
@abi87 abi87 requested a review from coffeeavax July 11, 2023 06:27
joshua-kim
joshua-kim reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@joshua-kim joshua-kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last few nits to remove the new line diffs + remove some of the unnecessary comments

snow/networking/router/main_test.go Outdated Show resolved Hide resolved
snow/networking/timeout/main_test.go Outdated Show resolved Hide resolved
vms/platformvm/block/builder/helpers_test.go Outdated Show resolved Hide resolved
vms/platformvm/block/builder/main_test.go Outdated Show resolved Hide resolved
vms/platformvm/main_test.go Outdated Show resolved Hide resolved
snow/networking/router/chain_router_test.go Show resolved Hide resolved
snow/networking/router/chain_router_test.go Outdated Show resolved Hide resolved
@abi87 abi87 requested a review from joshua-kim October 17, 2023 06:39
abi87
abi87 commented Oct 17, 2023
View reviewed changes
snow/networking/router/chain_router_test.go
@@ -325,6 +329,7 @@ func TestShutdownTimesOut(t *testing.T) {
// Ensure that a timeout fires if we don't get a response to a request
func TestRouterTimeout(t *testing.T) {
require := require.New(t)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the test have require in its own "block" separated by a line from the rest of the test

StephenButtolph
StephenButtolph approved these changes Oct 23, 2023
View reviewed changes
snow/networking/router/chain_router_test.go Outdated
@@ -1543,6 +1597,7 @@ func TestValidatorOnlyAllowedNodeMessageDrops(t *testing.T) {
"",
prometheus.NewRegistry(),
))
defer chainRouter.Shutdown(context.TODO())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we're ever planning on having a more canonical context in the tests - so normally we just use Background.

Suggested change
defer chainRouter.Shutdown(context.TODO())
defer chainRouter.Shutdown(context.Background())

(I think this applies a few times to this file)

@StephenButtolph StephenButtolph changed the title Some more goroutine leak fixed in UTs and prod code Shutdown TimeoutManager during node Shutdown Oct 23, 2023
@StephenButtolph StephenButtolph added this pull request to the merge queue Oct 23, 2023
Merged via the queue into dev with commit 4cd7051 Oct 23, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the fix-goroutine-leak branch October 23, 2023 16:11
joshua-kim added a commit that referenced this pull request Oct 27, 2023
@joshua-kim
Squashed commit of the following:
a4e9c00 
commit a4cee60
Author: Dan Laine <daniel.laine@avalabs.org>
Date:   Fri Oct 27 11:37:05 2023 -0400

    `merkledb` -- don't pass `BranchFactor` to `encodeDBNode` (#2217)

    Signed-off-by: Dan Laine <daniel.laine@avalabs.org>

commit cacbb9b
Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
Date:   Thu Oct 26 16:57:56 2023 -0400

    Move all blst function usage to `bls` pkg (#2222)

    Signed-off-by: Dan Laine <daniel.laine@avalabs.org>
    Co-authored-by: Dan Laine <daniel.laine@avalabs.org>

commit 3b213fc
Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
Date:   Thu Oct 26 16:53:26 2023 -0400

    Add json marshal tests to existing serialization tests in `platformvm/txs` pkg (#2227)

commit a6448ac
Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
Date:   Thu Oct 26 16:48:00 2023 -0400

    Update `golangci-lint` to `v1.55.1` (#2228)

commit fe74ed1
Author: Dan Laine <daniel.laine@avalabs.org>
Date:   Thu Oct 26 12:54:34 2023 -0400

    `merkledb` -- shift nit (#2218)

    Signed-off-by: Dan Laine <daniel.laine@avalabs.org>

commit e933587
Author: David Boehm <91908103+dboehm-avalabs@users.noreply.github.com>
Date:   Thu Oct 26 11:40:57 2023 -0400

    Reduce allocations on insert and remove (#2201)

    Signed-off-by: David Boehm <91908103+dboehm-avalabs@users.noreply.github.com>
    Signed-off-by: Dan Laine <daniel.laine@avalabs.org>
    Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org>
    Co-authored-by: Dan Laine <daniel.laine@avalabs.org>

commit 638000c
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Thu Oct 26 02:08:05 2023 -0400

    Update versions for v1.10.14 (#2225)

commit 8463690
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Thu Oct 26 00:12:05 2023 -0400

    Improve logging for block verification failure (#2224)

commit 787f0b6
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Wed Oct 25 17:35:17 2023 -0400

    Fix unexpected unlock (#2221)

commit 1f779af
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Wed Oct 25 17:13:25 2023 -0400

    Revert networking AllowConnection change (#2219)

commit f020a05
Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
Date:   Wed Oct 25 16:51:24 2023 -0400

    Add `TransferSubnetOwnershipTx` (#2178)

    Signed-off-by: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>

commit 150ffae
Author: Dan Laine <daniel.laine@avalabs.org>
Date:   Wed Oct 25 16:39:10 2023 -0400

    Add pebble database implementation (#1999)

    Co-authored-by: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

commit de168b1
Author: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Date:   Wed Oct 25 15:47:48 2023 -0400

    Add log for ungraceful shutdown on startup (#2215)

    Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>

commit cd77a1e
Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
Date:   Wed Oct 25 13:27:43 2023 -0400

    Remove `aggregate` struct (#2213)

commit 128757d
Author: Ceyhun Onur <ceyhun.onur@avalabs.org>
Date:   Wed Oct 25 02:03:45 2023 +0300

    Move the overridden manager into the node (#2199)

    Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>
    Co-authored-by: Alberto Benegiamo <alberto.benegiamo@gmail.com>
    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

commit 7903676
Author: Ceyhun Onur <ceyhun.onur@avalabs.org>
Date:   Tue Oct 24 20:44:01 2023 +0300

    Remove contains from validator manager interface (#2198)

    Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>
    Signed-off-by: Stephen Buttolph <stephen@avalabs.org>
    Co-authored-by: Alberto Benegiamo <alberto.benegiamo@gmail.com>
    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

commit 963aeb0
Author: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Date:   Tue Oct 24 12:54:31 2023 -0400

    Update TestDialContext to use ManuallyTrack (#2209)

    Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>

commit e337dda
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Tue Oct 24 12:42:05 2023 -0400

    Remove duplicate networking check (#2204)

    Signed-off-by: Dan Laine <daniel.laine@avalabs.org>
    Co-authored-by: Dan Laine <daniel.laine@avalabs.org>

commit 020e802
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Mon Oct 23 17:37:14 2023 -0400

    Add RSA max key length test (#2205)

commit d3287dd
Author: Alberto Benegiamo <alberto.benegiamo@gmail.com>
Date:   Mon Oct 23 12:09:10 2023 -0700

    Use custom codec for validator metadata (#1510)

commit 3a1dcca
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Mon Oct 23 13:47:37 2023 -0400

    Update local network readme (#2203)

commit 7b7931b
Author: Ceyhun Onur <ceyhun.onur@avalabs.org>
Date:   Mon Oct 23 20:36:47 2023 +0300

    Redesign validator set management to enable tracking all subnets (#1857)

    Signed-off-by: Ceyhun Onur <ceyhunonur54@gmail.com>
    Co-authored-by: Alberto Benegiamo <alberto.benegiamo@gmail.com>
    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

commit 4cd7051
Author: Alberto Benegiamo <alberto.benegiamo@gmail.com>
Date:   Mon Oct 23 08:48:56 2023 -0700

    Shutdown TimeoutManager during node Shutdown (#1707)

    Signed-off-by: Alberto Benegiamo <alberto.benegiamo@gmail.com>
    Co-authored-by: Dan Laine <daniel.laine@avalabs.org>
    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>
    Co-authored-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>

commit 6624270
Author: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Date:   Mon Oct 23 11:37:03 2023 -0400

    Add Heap Set (#2136)

    Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
    Signed-off-by: Stephen Buttolph <stephen@avalabs.org>
    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>
    Co-authored-by: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
    Co-authored-by: dboehm-avalabs <david.boehm@avalabs.org>
    Co-authored-by: David Boehm <91908103+dboehm-avalabs@users.noreply.github.com>
    Co-authored-by: Alberto Benegiamo <alberto.benegiamo@gmail.com>

commit 804f45b
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Fri Oct 20 14:16:44 2023 -0400

    Move selectStartGear lock from Handler into Engines (#2182)

commit 7ed450b
Author: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Date:   Fri Oct 20 09:55:40 2023 -0400

    Implement Heap Map (#2137)

    Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
    Co-authored-by: Alberto Benegiamo <alberto.benegiamo@gmail.com>
    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

commit a21d0cf
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Thu Oct 19 18:13:46 2023 -0400

    Move HealthCheck lock from Handler into Engines (#2173)

commit 26b1505
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Thu Oct 19 17:48:22 2023 -0400

    Move Shutdown lock from Handler into Engines (#2179)

commit 8c6b9d3
Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
Date:   Thu Oct 19 15:36:31 2023 -0400

    Add tests for BanffBlock serialization (#2194)

commit d9460de
Author: Dan Laine <daniel.laine@avalabs.org>
Date:   Thu Oct 19 15:26:16 2023 -0400

    Deprecate keystore config (#2195)

commit a9c260b
Author: David Boehm <91908103+dboehm-avalabs@users.noreply.github.com>
Date:   Thu Oct 19 14:44:51 2023 -0400

    Merkle db Make Paths only refer to lists of nodes (#2143)

    Signed-off-by: David Boehm <91908103+dboehm-avalabs@users.noreply.github.com>
    Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org>

commit 0faab95
Author: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Date:   Thu Oct 19 14:43:29 2023 -0400

    Update P2P proto docs (#2181)

    Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

commit 927c23d
Author: Dan Laine <daniel.laine@avalabs.org>
Date:   Thu Oct 19 13:07:46 2023 -0400

    Deprecate IPC configs (#2168)

    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

commit 3b843a3
Author: Stephen Buttolph <stephen@avalabs.org>
Date:   Thu Oct 19 12:14:58 2023 -0400

    Update cgo usage (#2184)

Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshua-kim joshua-kim joshua-kim approved these changes

@StephenButtolph StephenButtolph StephenButtolph approved these changes

@dhrubabasu dhrubabasu Awaiting requested review from dhrubabasu dhrubabasu is a code owner

@darioush darioush Awaiting requested review from darioush

Assignees

@abi87 abi87

Labels
cleanup Code quality improvement
Projects
No open projects
Ski Patrol
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@abi87 @darioush @joshua-kim @StephenButtolph @danlaine