Skip to content

Conversation

DaveCTurner
Copy link
Contributor

An exception here should be impossible, but we don't assert that, nor do
we emit a log message to prove it didn't happen in a production
environment. This commit adds the missing log and assert.

An exception here should be impossible, but we don't assert that, nor do
we emit a log message to prove it didn't happen in a production
environment. This commit adds the missing log and assert.
@DaveCTurner DaveCTurner requested a review from mhl-b July 17, 2025 09:02
@DaveCTurner DaveCTurner added >non-issue :Distributed Coordination/Network Http and internode communication implementations auto-backport Automatically create backport pull requests when merged v9.2.0 v9.1.1 v8.19.1 v8.18.5 labels Jul 17, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Jul 17, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@mhl-b
Copy link
Contributor

mhl-b commented Jul 17, 2025

I'm suspecting other place that might throw error that we dont catch. Async connect call that we dont check for exceptions. Looking at netty's code path I cannot tell 100% that channel can be null with exception as we assume in the code.

ChannelFuture connectFuture = bootstrapWithHandler.connect(); -> connect().await(timeout);

ChannelFuture connectFuture = bootstrapWithHandler.connect();
Channel channel = connectFuture.channel();
if (channel == null) {
ExceptionsHelper.maybeDieOnAnotherThread(connectFuture.cause());
throw new IOException(connectFuture.cause());
}

@DaveCTurner
Copy link
Contributor Author

Possibly, but that wouldn't cause a long-established channel to close. We can address that separately, no need to do so here.

@DaveCTurner
Copy link
Contributor Author

I opened #131520

Copy link
Contributor

@mhl-b mhl-b left a comment

Choose a reason for hiding this comment

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

LGTM

@DaveCTurner DaveCTurner merged commit a692cbd into elastic:main Jul 21, 2025
33 checks passed
@DaveCTurner DaveCTurner deleted the 2025/07/17/log-sendMessage-failure branch July 21, 2025 07:16
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Jul 21, 2025
An exception here should be impossible, but we don't assert that, nor do
we emit a log message to prove it didn't happen in a production
environment. This commit adds the missing log and assert.
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Jul 21, 2025
An exception here should be impossible, but we don't assert that, nor do
we emit a log message to prove it didn't happen in a production
environment. This commit adds the missing log and assert.
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
9.1
8.19
8.18 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 131418

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Jul 21, 2025
An exception here should be impossible, but we don't assert that, nor do
we emit a log message to prove it didn't happen in a production
environment. This commit adds the missing log and assert.

Backport of elastic#131418 to `8.18`
elasticsearchmachine pushed a commit that referenced this pull request Sep 1, 2025
An exception here should be impossible, but we don't assert that, nor do
we emit a log message to prove it didn't happen in a production
environment. This commit adds the missing log and assert.

Backport of #131418 to `8.18`
elasticsearchmachine pushed a commit that referenced this pull request Sep 1, 2025
An exception here should be impossible, but we don't assert that, nor do
we emit a log message to prove it didn't happen in a production
environment. This commit adds the missing log and assert.
elasticsearchmachine pushed a commit that referenced this pull request Sep 3, 2025
An exception here should be impossible, but we don't assert that, nor do
we emit a log message to prove it didn't happen in a production
environment. This commit adds the missing log and assert.
sarog pushed a commit to portsbuild/elasticsearch that referenced this pull request Sep 11, 2025
An exception here should be impossible, but we don't assert that, nor do
we emit a log message to prove it didn't happen in a production
environment. This commit adds the missing log and assert.
sarog pushed a commit to portsbuild/elasticsearch that referenced this pull request Sep 19, 2025
An exception here should be impossible, but we don't assert that, nor do
we emit a log message to prove it didn't happen in a production
environment. This commit adds the missing log and assert.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Distributed Coordination/Network Http and internode communication implementations >non-issue Team:Distributed Coordination Meta label for Distributed Coordination team v8.18.5 v8.19.1 v9.1.1 v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants