From 2fc68793f389ba74f0dc6f778a502a4adfa3a23d Mon Sep 17 00:00:00 2001 From: jholdstock Date: Mon, 14 Oct 2024 11:08:00 +0100 Subject: [PATCH 1/3] jsonrpc: Use process ctx as request ctx parent. This ensures that the context of in-flight RPC requests is cancelled when the dcrwallet process context is cancelled. --- dcrwallet.go | 2 +- internal/rpc/jsonrpc/server.go | 11 +++++++++-- rpcserver.go | 4 ++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/dcrwallet.go b/dcrwallet.go index 7013e9462..6d54dcda5 100644 --- a/dcrwallet.go +++ b/dcrwallet.go @@ -370,7 +370,7 @@ func run(ctx context.Context) error { // // Servers will be associated with a loaded wallet if it has already been // loaded, or after it is loaded later on. - gRPCServer, jsonRPCServer, err := startRPCServers(loader) + gRPCServer, jsonRPCServer, err := startRPCServers(ctx, loader) if err != nil { log.Errorf("Unable to create RPC servers: %v", err) return err diff --git a/internal/rpc/jsonrpc/server.go b/internal/rpc/jsonrpc/server.go index 7f43675c4..8e7cd37f6 100644 --- a/internal/rpc/jsonrpc/server.go +++ b/internal/rpc/jsonrpc/server.go @@ -1,5 +1,5 @@ // Copyright (c) 2013-2015 The btcsuite developers -// Copyright (c) 2017-2019 The Decred developers +// Copyright (c) 2017-2024 The Decred developers // Use of this source code is governed by an ISC // license that can be found in the LICENSE file. @@ -91,13 +91,20 @@ func jsonAuthFail(w http.ResponseWriter) { // NewServer creates a new server for serving JSON-RPC client connections, // both HTTP POST and websocket. -func NewServer(opts *Options, activeNet *chaincfg.Params, walletLoader *loader.Loader, listeners []net.Listener) *Server { +func NewServer(ctx context.Context, opts *Options, activeNet *chaincfg.Params, walletLoader *loader.Loader, listeners []net.Listener) *Server { serveMux := http.NewServeMux() const rpcAuthTimeoutSeconds = 10 server := &Server{ httpServer: http.Server{ Handler: serveMux, + // Use the provided context as the parent context for all requests + // to ensure handlers are able to react to both client disconnects + // as well as shutdown via the provided context. + BaseContext: func(l net.Listener) context.Context { + return ctx + }, + // Timeout connections which don't complete the initial // handshake within the allowed timeframe. ReadTimeout: time.Second * rpcAuthTimeoutSeconds, diff --git a/rpcserver.go b/rpcserver.go index 6e3870f90..7076a00b0 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -229,7 +229,7 @@ func generateClientKeyPair(caPriv any, ca *x509.Certificate) (cert, key []byte, return cert, key, nil } -func startRPCServers(walletLoader *loader.Loader) (*grpc.Server, *jsonrpc.Server, error) { +func startRPCServers(ctx context.Context, walletLoader *loader.Loader) (*grpc.Server, *jsonrpc.Server, error) { var jsonrpcAddrNotifier jsonrpcListenerEventServer var grpcAddrNotifier grpcListenerEventServer if cfg.RPCListenerEvents { @@ -367,7 +367,7 @@ func startRPCServers(walletLoader *loader.Loader) (*grpc.Server, *jsonrpc.Server TicketSplitAccount: cfg.TicketSplitAccount, Dial: cfg.dial, } - jsonrpcServer = jsonrpc.NewServer(&opts, activeNet.Params, walletLoader, listeners) + jsonrpcServer = jsonrpc.NewServer(ctx, &opts, activeNet.Params, walletLoader, listeners) for _, lis := range listeners { jsonrpcAddrNotifier.notify(lis.Addr().String()) } From 05105ec9baee7a2fcf794ad8386791ea2ba1a742 Mon Sep 17 00:00:00 2001 From: jholdstock Date: Mon, 14 Oct 2024 09:11:33 +0100 Subject: [PATCH 2/3] jsonrpc: Stop background rescans (more) gracefully Use the server waitgroup to ensure background rescans can return on their own terms rather than being forcefully terminated early. This is still not an ideal situation because in the case where the server/wallet is being shut down, RescanFromHeight will return with an "rpc connection closed" error (or similar) rather than returning because its provided context was cancelled. However, that is a notable improvement over the existing situation which could result in an unhandled panic. --- internal/rpc/jsonrpc/methods.go | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/internal/rpc/jsonrpc/methods.go b/internal/rpc/jsonrpc/methods.go index 53b17f5c8..37afae274 100644 --- a/internal/rpc/jsonrpc/methods.go +++ b/internal/rpc/jsonrpc/methods.go @@ -2032,9 +2032,14 @@ func (s *Server) importPrivKey(ctx context.Context, icmd any) (any, error) { } if rescan { - // TODO: This is not synchronized with process shutdown and - // will cause panics when the DB is closed mid-transaction. - go w.RescanFromHeight(context.Background(), n, scanFrom) + // Rescan in the background rather than blocking the rpc request. Use + // the server waitgroup to ensure the rescan can return cleanly rather + // than being killed mid database transaction. + s.wg.Add(1) + go func() { + defer s.wg.Done() + _ = w.RescanFromHeight(context.Background(), n, scanFrom) + }() } return nil, nil @@ -2084,9 +2089,14 @@ func (s *Server) importPubKey(ctx context.Context, icmd any) (any, error) { } if rescan { - // TODO: This is not synchronized with process shutdown and - // will cause panics when the DB is closed mid-transaction. - go w.RescanFromHeight(context.Background(), n, scanFrom) + // Rescan in the background rather than blocking the rpc request. Use + // the server waitgroup to ensure the rescan can return cleanly rather + // than being killed mid database transaction. + s.wg.Add(1) + go func() { + defer s.wg.Done() + _ = w.RescanFromHeight(context.Background(), n, scanFrom) + }() } return nil, nil @@ -2130,9 +2140,14 @@ func (s *Server) importScript(ctx context.Context, icmd any) (any, error) { } if rescan { - // TODO: This is not synchronized with process shutdown and - // will cause panics when the DB is closed mid-transaction. - go w.RescanFromHeight(context.Background(), n, scanFrom) + // Rescan in the background rather than blocking the rpc request. Use + // the server waitgroup to ensure the rescan can return cleanly rather + // than being killed mid database transaction. + s.wg.Add(1) + go func() { + defer s.wg.Done() + _ = w.RescanFromHeight(context.Background(), n, scanFrom) + }() } return nil, nil From 31c2f09f7b0ada069a1fb3beee163be34b5b90e0 Mon Sep 17 00:00:00 2001 From: jholdstock Date: Mon, 14 Oct 2024 14:29:31 +0100 Subject: [PATCH 3/3] jsonrpc: Rescan with server ctx not background ctx Using the server context instead of context.Background means that the long-lived rescan funcs are able to exit properly when the server is shutdown and its context is closed. --- internal/rpc/jsonrpc/methods.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/rpc/jsonrpc/methods.go b/internal/rpc/jsonrpc/methods.go index 37afae274..ceb8024ce 100644 --- a/internal/rpc/jsonrpc/methods.go +++ b/internal/rpc/jsonrpc/methods.go @@ -2038,7 +2038,8 @@ func (s *Server) importPrivKey(ctx context.Context, icmd any) (any, error) { s.wg.Add(1) go func() { defer s.wg.Done() - _ = w.RescanFromHeight(context.Background(), n, scanFrom) + serverCtx := s.httpServer.BaseContext(nil) + _ = w.RescanFromHeight(serverCtx, n, scanFrom) }() } @@ -2095,7 +2096,8 @@ func (s *Server) importPubKey(ctx context.Context, icmd any) (any, error) { s.wg.Add(1) go func() { defer s.wg.Done() - _ = w.RescanFromHeight(context.Background(), n, scanFrom) + serverCtx := s.httpServer.BaseContext(nil) + _ = w.RescanFromHeight(serverCtx, n, scanFrom) }() } @@ -2146,7 +2148,8 @@ func (s *Server) importScript(ctx context.Context, icmd any) (any, error) { s.wg.Add(1) go func() { defer s.wg.Done() - _ = w.RescanFromHeight(context.Background(), n, scanFrom) + serverCtx := s.httpServer.BaseContext(nil) + _ = w.RescanFromHeight(serverCtx, n, scanFrom) }() }