From f761d8589296ec0305e7a18bec0db02cab280384 Mon Sep 17 00:00:00 2001 From: Yuxuan 'fishy' Wang Date: Thu, 9 Jan 2025 10:40:30 -0800 Subject: [PATCH] thriftbp: Tweak client pool connection reuse logic Only exceptions defined in thrift IDL is safe for reuse (getting them means the roundtrip is done fully and the response is read fully). All other errors have different degree of risks for reusing. --- thriftbp/client_pool.go | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/thriftbp/client_pool.go b/thriftbp/client_pool.go index 6a84ab671..f0a1b74e0 100644 --- a/thriftbp/client_pool.go +++ b/thriftbp/client_pool.go @@ -711,22 +711,14 @@ func shouldCloseConnection(err error) bool { var te thrift.TException if errors.As(err, &te) { switch te.TExceptionType() { - case thrift.TExceptionTypeApplication, thrift.TExceptionTypeProtocol, thrift.TExceptionTypeTransport: - // We definitely should close the connection on TTransportException. - // We probably don't need to close the connection on TApplicationException - // and TProtocolException, but just close them to be safe, as the - // connection might be in some weird state when those errors happen. - return true case thrift.TExceptionTypeCompiled: - // For exceptions defined from the IDL, we definitely shouldn't close the - // connection. + // Exceptions defined in thrift IDL. + // Getting those means the roundtrip is done and response is read fully, + // so safe to reuse. + // This is the only non-nil error that the connection is safe for reuse return false } } - // We should avoid reusing the client if it hits a network error. - // We should also actively close the connection if it's a timeout, - // as this helps the server side to abandon the request early. - return errors.As(err, new(net.Error)) || - errors.Is(err, context.Canceled) || - errors.Is(err, context.DeadlineExceeded) + // Everything else has different degrees of risks of reusing the connection. + return true }