Skip to content

Keep safe response consumer when converting transport options - #1355

Open
RaphaelFakhri wants to merge 1 commit into
elastic:mainfrom
RaphaelFakhri:fix-options-safe-consumer
Open

RaphaelFakhri wants to merge 1 commit into
elastic:mainfrom
RaphaelFakhri:fix-options-safe-consumer

Conversation

@RaphaelFakhri

Copy link
Copy Markdown

Problem

RestClientOptions.of() and Rest5ClientOptions.of() convert any other TransportOptions implementation by starting from RequestOptions.DEFAULT. The converted options therefore use the plain response consumer factory instead of SafeResponseConsumer.DEFAULT_FACTORY, which initialOptions() uses to keep errors thrown while consuming a response from shutting down the I/O reactor.

This conversion runs on the default setup path. The ElasticsearchClient.of(...) builder methods transportOptions(fn) and retryConfig(...) produce DefaultTransportOptions, which the transport then converts:

ElasticsearchClient client = ElasticsearchClient.of(b -> b
    .host("https://localhost:9200")
    .apiKey(apiKey)
    .retryConfig(r -> r.maxRetries(3)) // or .transportOptions(o -> o.addHeader(...))
);
// client._transportOptions() no longer uses SafeResponseConsumer

The same happens with client.withTransportOptions(DefaultTransportOptions) or a custom TransportOptions. With either transport, an Error such as OutOfMemoryError raised while reading a response can then stop the I/O reactor and leave the client permanently unusable, the failure mode that SafeResponseConsumer was added to prevent (#1046, #1049).

Change

  • Start the conversion from SafeResponseConsumer.DEFAULT_REQUEST_OPTIONS in both RestClientOptions.of() and Rest5ClientOptions.of(), matching initialOptions().
  • Add a test to RestClientOptionsTest and Rest5ClientOptionsTest that builds a client with transportOptions(...) and checks that the resulting options use SafeResponseConsumer.DEFAULT_FACTORY.

Testing

./gradlew :java-client:test --tests co.elastic.clients.transport.rest_client.RestClientOptionsTest \
  --tests co.elastic.clients.transport.rest5_client.Rest5ClientOptionsTest
  • Without the fix: the 2 new tests fail (12 tests, 2 failed).
  • With the fix: all tests pass, together with SafeResponseConsumerTest (both transports) and ElasticsearchTransportConfigTest.
  • checkstyleMain, checkstyleTest, forbiddenApisMain and forbiddenApisTest pass.

Fixes #1354

RestClientOptions.of() and Rest5ClientOptions.of() built converted options
from RequestOptions.DEFAULT, so options set with the client builder's
transportOptions() or retryConfig(), or passed as DefaultTransportOptions,
lost the SafeResponseConsumer that prevents I/O reactor shutdowns. Start
from SafeResponseConsumer.DEFAULT_REQUEST_OPTIONS instead, as
initialOptions() does.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transport options set on the client builder drop SafeResponseConsumer

1 participant