Skip to content

fix(client): close unexpected task result stream #699

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.

Sign up for GitHub

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

Open
Ashfaqbs wants to merge 1 commit into
base: main
Choose a base branch
from
+97 −10
Open

fix(client): close unexpected task result stream #699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions ...docling-serve-client/src/main/java/ai/docling/serve/client/operations/TaskOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ai.docling.serve.client.operations;

import java.io.IOException;
import java.io.InputStream;

@edeandrea edeandrea Sep 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and this import is unused once closeQuietly goes:

Suggested change
import java.io.InputStream;

import java.nio.charset.StandardCharsets;

import ai.docling.serve.api.DoclingServeTaskApi;
Expand Down Expand Up @@ -36,17 +37,15 @@ public TaskOperations(HttpOperations httpOperations) {
* unique task identifier and optional wait time for polling.
* Must not be null.
* @return a {@link TaskStatusPollResponse} containing the current status of
* the task, its position in the queue, and any associated metadata.
* the task, its position in the queue, and any associated metadata.
* @throws IllegalArgumentException if the {@code request} is null.
*/
public TaskStatusPollResponse pollTaskStatus(TaskStatusPollRequest request) {
ValidationUtils.ensureNotNull(request, "request");

return this.httpOperations.executeGet(createRequestContext(
"/v1/status/poll/%s?wait=%d".formatted(
request.getTaskId(),
request.getWaitTime().toSeconds()),
TaskStatusPollResponse.class)
request.getTaskId(), request.getWaitTime().toSeconds()), TaskStatusPollResponse.class)
);
}

Expand All @@ -58,7 +57,7 @@ public TaskStatusPollResponse pollTaskStatus(TaskStatusPollRequest request) {
* @param request an instance of {@link TaskResultRequest} containing the unique task
* identifier. Must not be null.
* @return a {@link ConvertDocumentResponse} containing details about the converted
* document, processing time, status, and any associated errors or metadata.
* document, processing time, status, and any associated errors or metadata.
* @throws IllegalArgumentException if {@code request} is null.
*/
public ConvertDocumentResponse convertTaskResult(TaskResultRequest request) {
Expand All @@ -69,9 +68,9 @@ public ConvertDocumentResponse convertTaskResult(TaskResultRequest request) {
case HttpOperations.CONTENT_TYPE_JSON -> {
try (var is = response.getBody()) {
return httpOperations
.readValue(new String(is.readAllBytes(), StandardCharsets.UTF_8)
, ConvertDocumentResponse.class);
} catch (IOException e) {
.readValue(new String(is.readAllBytes(), StandardCharsets.UTF_8), ConvertDocumentResponse.class);
}
catch (IOException e) {
throw new DoclingServeClientException(e);
}
}
Expand All @@ -82,7 +81,10 @@ public ConvertDocumentResponse convertTaskResult(TaskResultRequest request) {
.inputStream(response.getBody())
.build();
}
default -> throw new DoclingServeClientException(null, "Invalid Content-Type in Task API response");
default -> {
closeQuietly(response.getBody());
throw new DoclingServeClientException(null, "Invalid Content-Type in Task API response");
}
Comment on lines +84 to +87

@edeandrea edeandrea Sep 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (non-blocking): try-with-resources would do the same job as closeQuietly and handle two edge cases better:

  • Failed close(): the Content-Type error is still what gets thrown, but the IOException from close() is attached to it as a suppressed exception instead of being dropped, so it still shows up in logs and stack traces.
  • null body: try-with-resources skips closing a null resource, so a custom HttpOperations that returns no body gets the Content-Type error. With closeQuietly it gets NullPointerException: Cannot invoke "java.io.InputStream.close()", which hides the real problem.

It also matches the CONTENT_TYPE_JSON branch above.

Suggested change
default -> {
closeQuietly(response.getBody());
throw new DoclingServeClientException(null, "Invalid Content-Type in Task API response");
}
default -> {
try (var ignored = response.getBody()) {
throw new DoclingServeClientException(null, "Invalid Content-Type in Task API response");
}
catch (IOException e) {
// never reached in practice: a close() failure is added as suppressed to the exception above
throw new DoclingServeClientException(e);
}
}

The catch is only there because InputStream.close() declares IOException. The one exception that leaves the try is the unchecked DoclingServeClientException, so the comment says it isn't a real code path.

}
}

Expand All @@ -96,14 +98,23 @@ public ConvertDocumentResponse convertTaskResult(TaskResultRequest request) {
* @param request an instance of {@link TaskResultRequest} containing the unique task
* identifier. Must not be null.
* @return a {@link ChunkDocumentResponse} containing details about the chunks,
* documents, processing time, and any associated metadata.
* documents, processing time, and any associated metadata.
* @throws IllegalArgumentException if {@code request} is null.
*/
public ChunkDocumentResponse chunkTaskResult(TaskResultRequest request) {
ValidationUtils.ensureNotNull(request, "request");
return this.httpOperations.executeGet(createRequestContext("/v1/result/%s".formatted(request.getTaskId()), ChunkDocumentResponse.class));
}

private static void closeQuietly(InputStream inputStream) {
try {
inputStream.close();
}
catch (IOException ignored) {
// the Content-Type error is the failure worth reporting
}
}

Comment on lines +109 to +117

@edeandrea edeandrea Sep 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you take the try-with-resources suggestion above, this helper is no longer needed:

Suggested change
private static void closeQuietly(InputStream inputStream) {
try {
inputStream.close();
}
catch (IOException ignored) {
// the Content-Type error is the failure worth reporting
}
}

private <O> RequestContext<Object, O> createRequestContext(String uri, Class<O> responseType) {
return RequestContext.<Object, O>builder()
.responseType(responseType)
Expand Down
76 changes: 76 additions & 0 deletions ...ng-serve-client/src/test/java/ai/docling/serve/client/operations/TaskOperationsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package ai.docling.serve.client.operations;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.io.ByteArrayInputStream;
import java.io.InputStream;

@edeandrea edeandrea Sep 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed by the test suggested below:

Suggested change
import java.io.InputStream;
import java.io.IOException;
import java.io.InputStream;

import java.util.Optional;
import java.util.concurrent.atomic.AtomicBoolean;

import org.junit.jupiter.api.Test;

import ai.docling.serve.api.task.request.TaskResultRequest;
import ai.docling.serve.client.DoclingServeClientException;

class TaskOperationsTests {
@Test
void convertTaskResultClosesBodyOnUnexpectedContentType() {
var closed = new AtomicBoolean();
var body = new ByteArrayInputStream(new byte[]{
1,
2,
3
}) {
@Override
public void close() {
closed.set(true);
}
};
var taskOperations = new TaskOperations(new StubHttpOperations(body, "text/html"));
var request = TaskResultRequest.builder().taskId("task-1").build();

assertThatThrownBy(() -> taskOperations.convertTaskResult(request))
.isInstanceOf(DoclingServeClientException.class)
.hasMessageContaining("Invalid Content-Type");
assertThat(closed).isTrue();
}

@edeandrea edeandrea Sep 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: a companion test for the failing-close() case. It fails on the current code (getSuppressed() is empty) and passes with the try-with-resources suggestion:

Suggested change
}
}
@Test
void convertTaskResultKeepsCloseFailureAsSuppressedOnUnexpectedContentType() {
var body = new ByteArrayInputStream(new byte[0]) {
@Override
public void close() throws IOException {
throw new IOException("close failed");
}
};
var taskOperations = new TaskOperations(new StubHttpOperations(body, "text/html"));
var request = TaskResultRequest.builder().taskId("task-1").build();
assertThatThrownBy(() -> taskOperations.convertTaskResult(request))
.isInstanceOf(DoclingServeClientException.class)
.hasMessageContaining("Invalid Content-Type")
.satisfies(t -> assertThat(t.getSuppressed())
.singleElement()
.isInstanceOf(IOException.class)
.hasFieldOrPropertyWithValue("message", "close failed"));
}


private static final class StubHttpOperations extends HttpOperations {
private final InputStream body;
private final String contentType;

StubHttpOperations(InputStream body, String contentType) {
this.body = body;
this.contentType = contentType;
}

@Override
protected <I> StreamResponse executeGetWithStreamResponse(RequestContext<I, StreamResponse> requestContext) {
return StreamResponse.builder()
.body(body)
.headers(name -> CONTENT_TYPE_HEADER.equals(name) ? Optional.of(contentType) : Optional.empty())
.build();
}

@Override
protected <I, O> O executeGet(RequestContext<I, O> requestContext) {
throw new UnsupportedOperationException();
}

@Override
protected <I, O> O executePost(RequestContext<I, O> requestContext) {
throw new UnsupportedOperationException();
}

@Override
protected <I> StreamResponse executePostWithStreamResponse(RequestContext<I, StreamResponse> requestContext) {
throw new UnsupportedOperationException();
}

@Override
protected <T> T readValue(String json, Class<T> valueType) {
throw new UnsupportedOperationException();
}
}
}