From 56032a078cc5a402633566e90d2d735546fe6fc8 Mon Sep 17 00:00:00 2001 From: David Walluck Date: Wed, 19 Aug 2026 10:21:05 -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 | 77 +++++++++++++++++-- .../koji/http/httpclient4/RetryUtilsTest.java | 74 ++++++++++++++++++ 3 files changed, 145 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..1776154 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,20 @@ */ 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.commons.lang3.Strings; +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.GET_TAG_ID; +import static com.redhat.red.build.koji.model.xmlrpc.messages.Constants.MULTI_CALL; public class RetryUtils { @@ -27,20 +36,30 @@ 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 static final int MAX_RETRY_COUNT = 10; 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 { + if ( retryCount > MAX_RETRY_COUNT ) + { + logger.warn( "Retry count {} exceeds maximum {}. Capping retries", retryCount, MAX_RETRY_COUNT ); + retryCount = MAX_RETRY_COUNT; + } + int count = 0; while ( true ) { @@ -55,15 +74,20 @@ 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.max( interval, Math.min( interval << ( count - 1 ), MAX_WAIT_SECONDS ) ); + logger.info( "{}: {}: Waiting {} seconds before next retry", cause.getClass().getName(), cause.getMessage(), seconds ); + try { - Thread.sleep( interval * 1000l ); + Thread.sleep( seconds * 1000L ); } catch ( InterruptedException ex ) { + Thread.currentThread().interrupt(); throw new RuntimeException( ex ); } } @@ -75,4 +99,43 @@ 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(); + + if ( multiCallObjs == null ) + { + return false; + } + + for ( KojiMultiCallObj multiCallObj : multiCallObjs ) + { + if ( !isSafeCall( multiCallObj.getMethodName() ) ) + { + return false; + } + } + + return true; + } + + return isSafeCall( method ); + } + + static boolean isSafeCall( String methodName ) + { + return !GET_TAG_ID.equals( methodName ) && Strings.CS.startsWithAny( methodName, "get", "list", "query", "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 ) ); + + } +}