Use proper executor for failing requests when connection closes by henningandersen · Pull Request #109236 · elastic/elasticsearch

henningandersen · 2024-05-31T08:54:52Z

Now use the executor declared by the handler rather than generic, since using generic is trappy wrt. deadlocks.

Now use the executor declared by the handler rather than generic, since using generic is trappy wrt. deadlocks. Closes elastic#109225

elasticsearchmachine · 2024-05-31T08:55:15Z

Hi @henningandersen, I've created a changelog YAML for you.

henningandersen · 2024-05-31T08:56:18Z

Opening as draft to see what CI and devs thinks before adding more tests.

henningandersen · 2024-05-31T09:45:53Z

ci/part-2 failure is #109237

…_on_closed_executor

henningandersen · 2024-05-31T13:06:42Z

This is now ready for reviews.

elasticsearchmachine · 2024-05-31T13:06:54Z

Pinging @elastic/es-distributed (Team:Distributed)

DaveCTurner

Overall approach looks good, I left a handful of smaller comments.

DaveCTurner · 2024-05-31T13:35:31Z

server/src/main/java/org/elasticsearch/transport/TransportService.java

+                if (enableStackOverflowAvoidance == false) {
+                    handler.handleException(exception);
+                } else {
+                    threadPool.generic().submit(new AbstractRunnable() {


I think I'd rather we used a ForkingResponseHandlerRunnable on both branches here, even though ofc GENERIC doesn't reject anything today so it doesn't really matter.

It is slightly annoying in that it asserts that the handler is not using direct executor, hence i did this route. I can make it work ofc, let me give it a shot.

Yeah sure I meant not for the direct case, just if enableStackOverflowAvoidance

Yeah, absolutely, the handler verification is still annoying, but made it slightly more optional when fixing this here:

f93cc1c

DaveCTurner · 2024-05-31T13:37:40Z

server/src/test/java/org/elasticsearch/transport/TransportServiceLifecycleTests.java

+                })), unusedReader(), executor)
+            );
+            nodeA.transportService.onConnectionClosed(connection);
+            expectThrows(ExecutionException.class, NodeDisconnectedException.class, () -> future.get(10, TimeUnit.SECONDS));


Suggest using an org.elasticsearch.action.support.ActionTestUtils#assertNoSuccessListener and one of the safeAwait or safeGet methods.

Yeah, that is better, see: f34ef8f

server/src/test/java/org/elasticsearch/transport/TransportServiceLifecycleTests.java

server/src/main/java/org/elasticsearch/transport/TransportService.java

DaveCTurner · 2024-05-31T14:59:02Z

server/src/main/java/org/elasticsearch/transport/ForkingResponseHandlerRunnable.java

+        @Nullable TransportException transportException,
+        Executor executorUsed
+    ) {
+        assert executorUsed != EsExecutors.DIRECT_EXECUTOR_SERVICE : "forking handler required, but got " + handler;


I see, yeah this LGTM

server/src/main/java/org/elasticsearch/transport/TransportService.java

server/src/test/java/org/elasticsearch/transport/TransportServiceLifecycleTests.java

DaveCTurner

LGTM

henningandersen · 2024-06-01T05:56:25Z

Thanks David!

When TransportService fails to send a transport action, it can complete the listener's `onFailure` with the `generic` executor. If the listener is a `PlainActionFuture` and also waits to be completed with a `generic` thread, it will trip the `assertCompleteAllowed` assertion. https://github.com/elastic/elasticsearch/blob/fb482f863d5430702b19bd3dd23e9d8652f12ddd/server/src/main/java/org/elasticsearch/transport/TransportService.java#L1062-L1064 With this PR, we no longer fork to the generic thread pool and instead just handle the exeption inline with the current thread. The expectation is that the downstream handler should take care potential stack overflow issues. This is similar to what is done in #109236

…#114375) When TransportService fails to send a transport action, it can complete the listener's `onFailure` with the `generic` executor. If the listener is a `PlainActionFuture` and also waits to be completed with a `generic` thread, it will trip the `assertCompleteAllowed` assertion. https://github.com/elastic/elasticsearch/blob/fb482f863d5430702b19bd3dd23e9d8652f12ddd/server/src/main/java/org/elasticsearch/transport/TransportService.java#L1062-L1064 With this PR, we no longer fork to the generic thread pool and instead just handle the exeption inline with the current thread. The expectation is that the downstream handler should take care potential stack overflow issues. This is similar to what is done in elastic#109236

#114493) When TransportService fails to send a transport action, it can complete the listener's `onFailure` with the `generic` executor. If the listener is a `PlainActionFuture` and also waits to be completed with a `generic` thread, it will trip the `assertCompleteAllowed` assertion. https://github.com/elastic/elasticsearch/blob/fb482f863d5430702b19bd3dd23e9d8652f12ddd/server/src/main/java/org/elasticsearch/transport/TransportService.java#L1062-L1064 With this PR, we no longer fork to the generic thread pool and instead just handle the exeption inline with the current thread. The expectation is that the downstream handler should take care potential stack overflow issues. This is similar to what is done in #109236

…#114375) When TransportService fails to send a transport action, it can complete the listener's `onFailure` with the `generic` executor. If the listener is a `PlainActionFuture` and also waits to be completed with a `generic` thread, it will trip the `assertCompleteAllowed` assertion. https://github.com/elastic/elasticsearch/blob/fb482f863d5430702b19bd3dd23e9d8652f12ddd/server/src/main/java/org/elasticsearch/transport/TransportService.java#L1062-L1064 With this PR, we no longer fork to the generic thread pool and instead just handle the exeption inline with the current thread. The expectation is that the downstream handler should take care potential stack overflow issues. This is similar to what is done in elastic#109236

…warning This setting is not getting removed in ES 9.0, so its usage should not generate a critical deprecation warning. `TransportServiceLifecycleTests` already uses `assertWarnings` instead of `assertCriticalWarnings` and we don't distinguish between warning levels in assertions (See elastic#80304) See elastic#109236 See ES-11224

See elastic#109236 See ES-11224

…warning (#124788) This setting is not getting removed in ES 9.0, so its usage should not generate a critical deprecation warning. `TransportServiceLifecycleTests` already uses `assertWarnings` instead of `assertCriticalWarnings` and we don't distinguish between warning levels in assertions (See #80304) See #109236 See ES-11224

This setting was supposed to be removed in 9.0, but turned to be non-critical. We should remove it in ES 10.0 See #109236 See ES-11224

Use proper executor for failing requests when connection closes

29cd7ad

Now use the executor declared by the handler rather than generic, since using generic is trappy wrt. deadlocks. Closes elastic#109225

henningandersen added >bug :Distributed/Network Http and internode communication implementations v8.15.0 labels May 31, 2024

Update docs/changelog/109236.yaml

df049ea

No more stack overflow avoidance

d279bfa

henningandersen added 2 commits May 31, 2024 14:18

fallback setting + tests.

7747639

Merge remote-tracking branch 'origin/main' into fix_transport_service…

d89ecb6

…_on_closed_executor

henningandersen marked this pull request as ready for review May 31, 2024 13:06

elasticsearchmachine added the Team:Distributed Meta label for distributed team. label May 31, 2024

henningandersen requested a review from DaveCTurner May 31, 2024 13:22

DaveCTurner reviewed May 31, 2024

View reviewed changes

henningandersen added 5 commits May 31, 2024 16:29

Use ForkingResponseHandlerRunnable

f93cc1c

Use assertNoSuccessListener

f34ef8f

Register setting.

1027967

Test for registered setting.

c4c3164

spotless

6a7ead0

henningandersen requested a review from DaveCTurner May 31, 2024 14:58

DaveCTurner reviewed May 31, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/transport/TransportService.java Outdated Show resolved Hide resolved

DaveCTurner reviewed May 31, 2024

View reviewed changes

bug and test name.

ff85285

DaveCTurner approved these changes May 31, 2024

View reviewed changes

henningandersen merged commit a5092e2 into elastic:main Jun 1, 2024

henningandersen mentioned this pull request Oct 9, 2024

Handle InternalSendException inline for non-forking handlers #114375

Merged

arteam mentioned this pull request Mar 13, 2025

[8.x] Downgrade transport.enable_stack_protection deprecation to a warning #124788

Merged

arteam added a commit to arteam/elasticsearch that referenced this pull request Mar 13, 2025

Mark transport.enable_stack_protection to be deprecated in v10

13e06a8

See elastic#109236 See ES-11224

arteam mentioned this pull request Mar 13, 2025

Mark transport.enable_stack_protection to be removed in v10 #124804

Merged

arteam added a commit that referenced this pull request Mar 14, 2025

Mark transport.enable_stack_protection to be removed in v10

a49fa0b

This setting was supposed to be removed in 9.0, but turned to be non-critical. We should remove it in ES 10.0 See #109236 See ES-11224

Conversation

henningandersen commented May 31, 2024

Uh oh!

elasticsearchmachine commented May 31, 2024

Uh oh!

henningandersen commented May 31, 2024

Uh oh!

henningandersen commented May 31, 2024

Uh oh!

henningandersen commented May 31, 2024

Uh oh!

elasticsearchmachine commented May 31, 2024

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 31, 2024

Choose a reason for hiding this comment

Uh oh!

henningandersen May 31, 2024

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 31, 2024

Choose a reason for hiding this comment

Uh oh!

henningandersen May 31, 2024

Choose a reason for hiding this comment

Uh oh!

DaveCTurner May 31, 2024

Choose a reason for hiding this comment

Uh oh!

henningandersen May 31, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner May 31, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen commented Jun 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants