Skip to content

Commit 8177d0b

Browse files
buehlmannBenjamin Bühlmann
authored andcommitted
Release pooled connection after DELETE to prevent pool exhaustion
AbstractApi.delete(...) returned the JAX-RS Response without reading its entity or closing it. Most delete callers are void (e.g. GroupApi.deleteGroup, ProjectApi.deleteProject) and discard the returned Response, so with a pooling/Apache connector (used for proxied clients) the underlying connection is never returned to the pool. GitLabs delete endpoints answer 200/202 with a body, so each such delete leaks one pooled connection; with the default of 2 connections per route the pool is quickly exhausted and subsequent requests block indefinitely in AbstractConnPool.getPoolEntryBlocking. This issue was not visible before #1312: setting client properties per request kept the ClientConfig RequestScoped, so the connector (and its pool) was rebuilt per request and any leaked connection was discarded with it. Since #1312 the ClientConfig is a singleton and the pool is shared, so the leak now accumulates and exhausts the pool. Buffer the delete body via Response.bufferEntity() in the delete() helpers. This releases the connection immediately while keeping the Response readable for the few callers that consume it (LicenseApi.deleteLicense, IssuesApi.deleteIssueLink, EpicsApi.removeIssue). get/post/put are unaffected since those callers already release the connection via readEntity().
1 parent ad73110 commit 8177d0b

2 files changed

Lines changed: 101 additions & 2 deletions

File tree

gitlab4j-api/src/main/java/org/gitlab4j/api/AbstractApi.java

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -696,7 +696,7 @@ protected Response delete(
696696
Response.Status expectedStatus, MultivaluedMap<String, String> queryParams, Object... pathArgs)
697697
throws GitLabApiException {
698698
try {
699-
return validate(getApiClient().delete(queryParams, pathArgs), expectedStatus);
699+
return releaseConnection(validate(getApiClient().delete(queryParams, pathArgs), expectedStatus));
700700
} catch (Exception e) {
701701
throw handle(e);
702702
}
@@ -715,12 +715,38 @@ protected Response delete(
715715
protected Response delete(Response.Status expectedStatus, MultivaluedMap<String, String> queryParams, URL url)
716716
throws GitLabApiException {
717717
try {
718-
return validate(getApiClient().delete(queryParams, url), expectedStatus);
718+
return releaseConnection(validate(getApiClient().delete(queryParams, url), expectedStatus));
719719
} catch (Exception e) {
720720
throw handle(e);
721721
}
722722
}
723723

724+
/**
725+
* Releases the underlying HTTP connection of a DELETE response back to the connection pool.
726+
*
727+
* <p>Most {@code delete(...)} callers (e.g. {@code GroupApi#deleteGroup},
728+
* {@code ProjectApi#deleteProject}) are {@code void} and discard the returned {@link Response}
729+
* without closing it. When GitLabApi is configured with a pooling/Apache connector (for example
730+
* via a proxy), a {@code Response} whose entity is never read or closed keeps its connection
731+
* leased and never returns it to the pool. GitLab's delete endpoints typically answer 202/200
732+
* <em>with a body</em>, so these connections leak and the pool (default 2 per route) is
733+
* exhausted, after which further requests block in {@code getPoolEntryBlocking}.
734+
*
735+
* <p>{@link Response#bufferEntity()} reads the body into memory and releases the
736+
* connection immediately, while keeping the {@code Response} fully readable for the few callers
737+
* that do consume the body (e.g. {@code LicenseApi#deleteLicense}, {@code IssuesApi#deleteIssueLink},
738+
* {@code EpicsApi#removeIssue}).
739+
*
740+
* @param response the validated DELETE response
741+
* @return the same response, with its entity buffered and connection released
742+
*/
743+
private Response releaseConnection(Response response) {
744+
if (response.hasEntity()) {
745+
response.bufferEntity();
746+
}
747+
return response;
748+
}
749+
724750
/**
725751
* Convenience method for adding query and form parameters to a get() or post() call.
726752
*
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
package org.gitlab4j.api;
2+
3+
import static org.junit.jupiter.api.Assertions.assertSame;
4+
import static org.mockito.ArgumentMatchers.any;
5+
import static org.mockito.ArgumentMatchers.nullable;
6+
import static org.mockito.Mockito.mock;
7+
import static org.mockito.Mockito.never;
8+
import static org.mockito.Mockito.verify;
9+
import static org.mockito.Mockito.when;
10+
11+
import jakarta.ws.rs.core.MultivaluedMap;
12+
import jakarta.ws.rs.core.Response;
13+
14+
import org.junit.jupiter.api.Test;
15+
import org.junit.jupiter.api.extension.ExtendWith;
16+
import org.mockito.junit.jupiter.MockitoExtension;
17+
18+
/**
19+
* Verifies that {@link AbstractApi#delete} releases its connection back to the pool.
20+
*
21+
* <p>Most delete callers are {@code void} and discard the returned {@link Response} without closing
22+
* it; with a pooling/Apache connector that leaks the connection and eventually exhausts the pool
23+
* (requests then block in {@code getPoolEntryBlocking}). The fix buffers the delete body,
24+
* which releases the connection while keeping the response readable for callers that consume it.
25+
*/
26+
@ExtendWith(MockitoExtension.class)
27+
public class TestAbstractApiDelete {
28+
29+
private static class TestApi extends AbstractApi {
30+
TestApi(GitLabApi gitLabApi) {
31+
super(gitLabApi);
32+
}
33+
}
34+
35+
private TestApi setupApi(Response response) throws Exception {
36+
GitLabApi gitLabApi = mock(GitLabApi.class);
37+
GitLabApiClient apiClient = mock(GitLabApiClient.class);
38+
39+
when(gitLabApi.getApiClient()).thenReturn(apiClient);
40+
when(apiClient.delete(nullable(MultivaluedMap.class), any(Object[].class))).thenReturn(response);
41+
when(apiClient.validateSecretToken(response)).thenReturn(true);
42+
when(response.getStatus()).thenReturn(Response.Status.OK.getStatusCode());
43+
44+
return new TestApi(gitLabApi);
45+
}
46+
47+
@Test
48+
public void shouldBufferEntityToReleaseConnectionWhenBodyPresent() throws Exception {
49+
Response response = mock(Response.class);
50+
when(response.hasEntity()).thenReturn(true);
51+
52+
TestApi api = setupApi(response);
53+
Response result = api.delete(Response.Status.OK, null, "groups", 1L);
54+
55+
assertSame(response, result);
56+
57+
// The connection is released via bufferEntity() even though void callers discard the Response,
58+
// and the Response stays readable (not closed) for callers that read the deleted entity.
59+
verify(response).bufferEntity();
60+
verify(response, never()).close();
61+
}
62+
63+
@Test
64+
public void shouldNotBufferWhenNoEntity() throws Exception {
65+
Response response = mock(Response.class);
66+
when(response.hasEntity()).thenReturn(false);
67+
68+
TestApi api = setupApi(response);
69+
api.delete(Response.Status.OK, null, "groups", 1L);
70+
71+
verify(response, never()).bufferEntity();
72+
}
73+
}

0 commit comments

Comments
 (0)