From 14f1bba2eceaa6cba6c7d85e39b59a667106734d Mon Sep 17 00:00:00 2001 From: David Walluck Date: Mon, 30 Mar 2026 15:23:42 -0400 Subject: [PATCH] NCL-6509: Improve XML-RPC retry handling Extend the XML-RPC retry logic to retry on `NoHttpResponseException` in addition to `ConnectException`. Since we can't know in the case of a `NoHttpResponseException` whether the server processed the request, we check the method name to determine if it is safe to retry. We assume that API calls starting with the words get, list, query, or has are read-only and safe to retry and all others are not. Change from linear backoff to exponential backoff to give the server more time. --- .../http/httpclient4/HC4SyncObjectClient.java | 2 +- .../koji/http/httpclient4/RetryUtils.java | 61 +++++++++++++-- .../koji/http/httpclient4/RetryUtilsTest.java | 74 +++++++++++++++++++ 3 files changed, 129 insertions(+), 8 deletions(-) create mode 100644 src/test/java/com/redhat/red/build/koji/http/httpclient4/RetryUtilsTest.java diff --git a/src/main/java/com/redhat/red/build/koji/http/httpclient4/HC4SyncObjectClient.java b/src/main/java/com/redhat/red/build/koji/http/httpclient4/HC4SyncObjectClient.java index d33e082..ede97dd 100644 --- a/src/main/java/com/redhat/red/build/koji/http/httpclient4/HC4SyncObjectClient.java +++ b/src/main/java/com/redhat/red/build/koji/http/httpclient4/HC4SyncObjectClient.java @@ -57,7 +57,7 @@ public HC4SyncObjectClient( final HttpFactory httpFactory, final SiteConfig site public T call( final Object request, final Class responseType, final UrlBuilder urlBuilder, final RequestModifier requestModifier ) throws XmlRpcException { - return RetryUtils.withRetry( () -> doCall( request, responseType, urlBuilder, requestModifier ) ); + return RetryUtils.withRetry( () -> doCall( request, responseType, urlBuilder, requestModifier ), request ); } private T doCall( final Object request, final Class responseType, final UrlBuilder urlBuilder, diff --git a/src/main/java/com/redhat/red/build/koji/http/httpclient4/RetryUtils.java b/src/main/java/com/redhat/red/build/koji/http/httpclient4/RetryUtils.java index 2a45da0..6ca6fd2 100644 --- a/src/main/java/com/redhat/red/build/koji/http/httpclient4/RetryUtils.java +++ b/src/main/java/com/redhat/red/build/koji/http/httpclient4/RetryUtils.java @@ -15,11 +15,18 @@ */ package com.redhat.red.build.koji.http.httpclient4; +import com.redhat.red.build.koji.model.xmlrpc.KojiMultiCallObj; +import com.redhat.red.build.koji.model.xmlrpc.messages.MultiCallRequest; +import org.apache.http.NoHttpResponseException; +import org.commonjava.rwx.anno.Request; import org.commonjava.rwx.error.XmlRpcException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.net.ConnectException; +import java.util.List; + +import static com.redhat.red.build.koji.model.xmlrpc.messages.Constants.MULTI_CALL; public class RetryUtils { @@ -27,19 +34,21 @@ public class RetryUtils public static final int DEFAULT_RETRY_COUNT = 3; - public static final long DEFAULT_WAIT_SECONDS = 10; + public static final long DEFAULT_WAIT_SECONDS = 10L; + + public static final long MAX_WAIT_SECONDS = 60L; public interface CallToRetry { R process() throws XmlRpcException; } - public static T withRetry( CallToRetry call ) throws XmlRpcException + public static T withRetry( CallToRetry call, Object request ) throws XmlRpcException { - return withRetry( DEFAULT_RETRY_COUNT, DEFAULT_WAIT_SECONDS, call ); + return withRetry( DEFAULT_RETRY_COUNT, DEFAULT_WAIT_SECONDS, call, request ); } - public static T withRetry( int retryCount, long interval, CallToRetry call ) throws XmlRpcException + public static T withRetry( int retryCount, long interval, CallToRetry call, Object request ) throws XmlRpcException { int count = 0; while ( true ) @@ -55,12 +64,16 @@ public static T withRetry( int retryCount, long interval, CallToRetry call ) throw e; } - if ( e.getCause() instanceof ConnectException ) + Throwable cause = e.getCause(); + + if ( cause instanceof ConnectException || ( cause instanceof NoHttpResponseException && isSafeToRetry( request ) ) ) { - logger.info( "ConnectException {}/{}, Waiting for {} second(s) before next retry ...", e.getCause().getClass(), e.getCause().getMessage(), interval ); + long seconds = Math.min( interval << ( count - 1 ), MAX_WAIT_SECONDS ); + logger.info( "ConnectException {}/{}, Waiting for {} second(s) before next retry ...", cause.getClass(), cause.getMessage(), seconds ); + try { - Thread.sleep( interval * 1000l ); + Thread.sleep( seconds * 1000L ); } catch ( InterruptedException ex ) { @@ -75,4 +88,38 @@ public static T withRetry( int retryCount, long interval, CallToRetry call ) } } + static boolean isSafeToRetry( Object obj ) + { + Request request = obj.getClass().getAnnotation( Request.class ); + + if ( request == null ) + { + return false; + } + + String method = request.method(); + + if ( MULTI_CALL.equals( method ) ) + { + MultiCallRequest multiCallRequest = (MultiCallRequest) obj; + List multiCallObjs = multiCallRequest.getMultiCallObjs(); + + for ( KojiMultiCallObj multiCallObj : multiCallObjs ) + { + if ( !isSafeCall( multiCallObj.getMethodName() ) ) + { + return false; + } + } + + return true; + } + + return isSafeCall( method ); + } + + static boolean isSafeCall( String methodName ) + { + return methodName.startsWith( "get" ) || methodName.startsWith( "list" ) || methodName.startsWith( "query" ) || methodName.startsWith( "has" ); + } } diff --git a/src/test/java/com/redhat/red/build/koji/http/httpclient4/RetryUtilsTest.java b/src/test/java/com/redhat/red/build/koji/http/httpclient4/RetryUtilsTest.java new file mode 100644 index 0000000..9197351 --- /dev/null +++ b/src/test/java/com/redhat/red/build/koji/http/httpclient4/RetryUtilsTest.java @@ -0,0 +1,74 @@ +/* + * Copyright (C) 2015 Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.redhat.red.build.koji.http.httpclient4; + +import com.redhat.red.build.koji.KojiClientUtils; +import com.redhat.red.build.koji.model.xmlrpc.messages.AllPermissionsRequest; +import com.redhat.red.build.koji.model.xmlrpc.messages.ApiVersionRequest; +import com.redhat.red.build.koji.model.xmlrpc.messages.CGInlinedImportRequest; +import com.redhat.red.build.koji.model.xmlrpc.messages.CGUploadedImportRequest; +import com.redhat.red.build.koji.model.xmlrpc.messages.CheckPermissionRequest; +import com.redhat.red.build.koji.model.xmlrpc.messages.Constants; +import com.redhat.red.build.koji.model.xmlrpc.messages.GetBuildTypeRequest; +import com.redhat.red.build.koji.model.xmlrpc.messages.ListBuildTypesRequest; +import com.redhat.red.build.koji.model.xmlrpc.messages.LoginRequest; +import com.redhat.red.build.koji.model.xmlrpc.messages.LoginResponse; +import com.redhat.red.build.koji.model.xmlrpc.messages.LogoutRequest; +import com.redhat.red.build.koji.model.xmlrpc.messages.MultiCallRequest; +import com.redhat.red.build.koji.model.xmlrpc.messages.QueryRpmSigsRequest; +import org.junit.Test; + +import java.util.Collections; +import java.util.List; + +import static org.commonjava.rwx.vocab.Nil.NIL_VALUE; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +public class RetryUtilsTest +{ + @Test + public void testIsSafeToRetry() + { + assertThat( RetryUtils.isSafeToRetry( new AllPermissionsRequest() ), is( true ) ); + assertThat( RetryUtils.isSafeToRetry( new ApiVersionRequest() ), is( true ) ); + assertThat( RetryUtils.isSafeToRetry( new CheckPermissionRequest() ), is( true ) ); + assertThat( RetryUtils.isSafeToRetry( new GetBuildTypeRequest() ), is( true ) ); + assertThat( RetryUtils.isSafeToRetry( new ListBuildTypesRequest() ), is( true ) ); + assertThat( RetryUtils.isSafeToRetry( new QueryRpmSigsRequest() ), is( true ) ); + } + + @Test + public void testIsNotSafeToRetry() + { + assertThat( RetryUtils.isSafeToRetry( new CGInlinedImportRequest() ), is( false ) ); + assertThat( RetryUtils.isSafeToRetry( new CGUploadedImportRequest() ), is( false ) ); + assertThat( RetryUtils.isSafeToRetry( new LoginRequest() ), is( false ) ); + assertThat( RetryUtils.isSafeToRetry( new LogoutRequest() ), is( false ) ); + assertThat( RetryUtils.isSafeToRetry( new LoginResponse() ), is( false ) ); + } + + @Test + public void testRetryWithMultiCall() + { + List args = Collections.singletonList( NIL_VALUE ); + MultiCallRequest req1 = KojiClientUtils.buildMultiCallRequest( Constants.GET_API_VERSION, args ); + assertThat( RetryUtils.isSafeToRetry( req1 ), is( true ) ); + MultiCallRequest req2 = KojiClientUtils.buildMultiCallRequest( Constants.LOGOUT, args ); + assertThat( RetryUtils.isSafeToRetry( req2 ), is( false ) ); + + } +}