Skip to content

rfq+server: properly configure gRPC keep alive settings for client+server #1834

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

rfq+server: properly configure gRPC keep alive settings for client+server #1834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3132ddc
rfq: add client-side keepalive to price oracle connections
Roasbeef Oct 3, 2025
9976595
server: enhance gRPC keepalive configuration
Roasbeef Oct 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions rfq/oracle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/keepalive"
)

// PriceQueryIntent is an enum that represents the intent of a price rate
Expand Down Expand Up @@ -196,6 +197,21 @@ func serverDialOpts() ([]grpc.DialOption, error) {
transportCredentials := credentials.NewTLS(&tlsConfig)
opts = append(opts, grpc.WithTransportCredentials(transportCredentials))

// Configure client-side keepalive parameters. These settings ensure
// the client actively probes the connection health and prevents idle
// connections from being silently closed by intermediaries or the
// server.
opts = append(opts, grpc.WithKeepaliveParams(keepalive.ClientParameters{
// Ping server after 30 seconds of inactivity.
Time: 30 * time.Second,
// Wait 20 seconds for ping response.
Timeout: 20 * time.Second,
// Permit keepalive pings even when there are no active
// streams. This is critical for long-lived connections with
// infrequent RFQ requests.
PermitWithoutStream: true,
}))

return opts, nil
}

Expand All @@ -209,6 +225,21 @@ func insecureServerDialOpts() ([]grpc.DialOption, error) {
insecure.NewCredentials(),
))

// Configure client-side keepalive parameters. These settings ensure
// the client actively probes the connection health and prevents idle
// connections from being silently closed by intermediaries or the
// server.
opts = append(opts, grpc.WithKeepaliveParams(keepalive.ClientParameters{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do we want this to be a pure copy-paste of the opts on lines 200-214? Can't we reuse?

Copy link
Member

Choose a reason for hiding this comment

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

+1, let's extract some consts or the whole configuration and re-use it

// Ping server after 30 seconds of inactivity.
Time: 30 * time.Second,
// Wait 20 seconds for ping response.
Timeout: 20 * time.Second,
// Permit keepalive pings even when there are no active
// streams. This is critical for long-lived connections with
// infrequent RFQ requests.
PermitWithoutStream: true,
}))
Comment on lines +228 to +241
Copy link

Choose a reason for hiding this comment

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

medium

This block of code for configuring keep-alive parameters is identical to the one in serverDialOpts() (lines 200-213). To avoid duplication and improve maintainability, consider extracting these options into a package-level variable.

For example, you could define:

var clientKeepaliveDialOption = grpc.WithKeepaliveParams(keepalive.ClientParameters{
	Time:                30 * time.Second,
	Timeout:             20 * time.Second,
	PermitWithoutStream: true,
})

And then use it in both serverDialOpts() and insecureServerDialOpts() like this:

opts = append(opts, clientKeepaliveDialOption)

This would make the code cleaner and easier to modify in the future.


return opts, nil
}

Expand Down
33 changes: 28 additions & 5 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,11 +370,34 @@ func (s *Server) RunUntilShutdown(mainErrChan <-chan error) error {
serverOpts = append(serverOpts, rpcServerOpts...)
serverOpts = append(serverOpts, ServerMaxMsgReceiveSize)

keepAliveParams := keepalive.ServerParameters{
MaxConnectionIdle: time.Minute * 2,
}

serverOpts = append(serverOpts, grpc.KeepaliveParams(keepAliveParams))
// Configure server-side keepalive parameters. These settings allow the
// server to actively probe the connection health and ensure connections
// stay alive during idle periods.
serverKeepalive := keepalive.ServerParameters{
// Ping client after 1 minute of inactivity.
Time: time.Minute,
Copy link
Member

Choose a reason for hiding this comment

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

isn't it redundant to also have the server pinging the client?

Why not just have it be the responsibility of the client

Copy link
Member Author

Choose a reason for hiding this comment

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

This mirrors what lnd does,and we havne't had any issues with it. I view it as defense in depth. Can you think of a reason where the server also pinging will somehow disrupt the connection?

// Wait 20 seconds for ping response.
Timeout: 20 * time.Second,
// Allow connections to remain idle for extended periods. This
// is particularly important for RFQ operations where price
// oracle connections may be idle for long periods.
MaxConnectionIdle: time.Hour * 24,
}

// Configure client enforcement policy. This allows clients to send
// keepalive pings even when there are no active streams, which is
// crucial for long-lived connections with infrequent activity.
clientKeepalive := keepalive.EnforcementPolicy{
// Minimum time between client pings.
MinTime: 5 * time.Second,
// Allow pings without active RPCs.
PermitWithoutStream: true,
}

serverOpts = append(
Copy link
Member

Choose a reason for hiding this comment

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

This server is the tapd rpcserver. The oracle server is actually not implemented here so this has been misplaced. We could add it to the reference implementation in docs/examples/basic-price-oracle.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is misplaced. The intent is that we resolve this on the tapd level as well, as we have other streaming calls, and no keep alive, so possible we run into this issue in other instances.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add it to the example oracle as well.

serverOpts, grpc.KeepaliveParams(serverKeepalive),
grpc.KeepaliveEnforcementPolicy(clientKeepalive),
)

grpcServer := grpc.NewServer(serverOpts...)
defer grpcServer.Stop()
Expand Down