-
Notifications
You must be signed in to change notification settings - Fork 12
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.
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
kwild,rpc: db and rpc timeout settings #736
Conversation
Resolves #717 , although this resolution is not DB specific. The timeout is applied at the level of the RPC service request. The linked issue suggests that solution is preferred, and I think so too. |
jsonrpc: timeout kwild,jsonrpc: use RPC timeout setting
Added the service-level DB read timeout ( The RPC servers themselves also have a |
@@ -555,6 +557,8 @@ func DefaultConfig() *KwildConfig { | |||
DBPort: "5432", // ignored with unix socket, but applies if IP used for DBHost | |||
DBUser: "kwild", | |||
DBName: "kwild", | |||
RPCTimeout: Duration(45 * time.Second), | |||
ReadTxTimeout: Duration(5 * time.Second), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was very surprised to find that the default for the db read timeout was 100ms on v0.6.x. Isn't that extremely short? Shortest I would consider is probably 500ms-2s
Also note that the server timeout is separate, much longer, applies to all requests, and is primarily a defensive mechanism for the server.
Regarding the gRPC request timeout, it really does look like you have to make a unary interceptor like this for per-request timeout: grpc/grpc-go#5059 |
yeah, now I see what you mean |
WIP: looks fine, but need to test e2e.
This adds two timeouts with settings to kwild:
app.rpc_timeout
-- timeout for the duration of request handling for both gRPC (and thus http gateway) and JSON-RPC servers enforce the timeout. This is a server timeout.app.db_read_timeout
-- timeout on read-only DB actions (actually engine, but using the read-only DB tx). This is a service timeout applied only in Call and Query methods of the user service.For gRCP,
rpc_timeout
uses a unary interceptor that cancels the context given to the service method. If I missed an existing timeout feature in the grpc packages, please let me know. I feel like this should have already existed.For JSON-RPC, I thought this was handled by
http.Server.WriteTimeout
, but shockingly that timeout only closes the connection without cancelling thehttp.Request
's context. This change employs thehttp.TimeoutHandler
in the standard library to do this safely without concurrent writes to thehttp.ResponseWriter
.