You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/04/29 06:25:41 UTC
[GitHub] [pulsar] lhotari opened a new pull request, #15382: [Broker/Client] Close connection if a ping message cannot be sent
lhotari opened a new pull request, #15382:
URL: https://github.com/apache/pulsar/pull/15382
### Motivation
The keep alive implementation used in the Broker and the Java client ignores any failures that happen while writing the Ping message.
In Netty 4+, it's necessary to add a listener to detect failures when writing to outbound channels.
The connection should be closed if a ping message cannot be sent. The future is notified once the OS level TCP/IP stack accepts the message for delivery.
This change will help detect the situation where writing to the outbound channel returns EBADF errno from the OS level and the connection isn't yet completely closed.
EBADF errno maps to java.nio.channels.ClosedChannelException which has this kind of explanation:
> Checked exception thrown when an attempt is made to invoke or complete an I/O operation upon channel that is closed, or at least closed to that operation. That this exception is thrown does not necessarily imply that the channel is completely closed. A socket channel whose write half has been shut down, for example, may still be open for reading.
### Modifications
Add a listener that closes the connection if sending the Ping message fails.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #15382: [Broker/Client] Close connection if a ping message cannot be sent
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15382:
URL: https://github.com/apache/pulsar/pull/15382#discussion_r861556999
##########
pulsar-common/src/main/java/org/apache/pulsar/common/protocol/PulsarHandler.java:
##########
@@ -110,7 +110,14 @@ private void handleKeepAliveTimeout() {
log.debug("[{}] Sending ping message", ctx.channel());
}
waitingForPingResponse = true;
- ctx.writeAndFlush(Commands.newPing());
+ ctx.writeAndFlush(Commands.newPing())
+ .addListener(future -> {
+ if (!future.isSuccess()) {
+ log.warn("[{}] Forcing connection to close since cannot send a ping message.",
+ ctx.channel(), future.cause());
+ ctx.close();
Review Comment:
It is, but it would be more consistent to use `future.channel().close()` in this case.
One detail is that in Netty 4+, the listeners get executed in the event loop. Explained in https://netty.io/wiki/new-and-noteworthy-in-4.0.html#well-defined-thread-model .
> The ChannelFutureListeners added to ChannelFuture are always invoked by the EventLoop thread assigned to the future's associated Channel.
However calling `ctx.close()` is also safe outside the event loop so that wouldn't make a difference in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15382: [Broker/Client] Close connection if a ping message cannot be sent
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15382:
URL: https://github.com/apache/pulsar/pull/15382#discussion_r861530983
##########
pulsar-common/src/main/java/org/apache/pulsar/common/protocol/PulsarHandler.java:
##########
@@ -110,7 +110,14 @@ private void handleKeepAliveTimeout() {
log.debug("[{}] Sending ping message", ctx.channel());
}
waitingForPingResponse = true;
- ctx.writeAndFlush(Commands.newPing());
+ ctx.writeAndFlush(Commands.newPing())
+ .addListener(future -> {
+ if (!future.isSuccess()) {
+ log.warn("[{}] Forcing connection to close since cannot send a ping message.",
+ ctx.channel(), future.cause());
+ ctx.close();
Review Comment:
Is it safe to call `ctx.close` in the listener?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #15382: [Broker/Client] Close connection if a ping message cannot be sent
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15382:
URL: https://github.com/apache/pulsar/pull/15382#discussion_r861550694
##########
pulsar-common/src/main/java/org/apache/pulsar/common/protocol/PulsarHandler.java:
##########
@@ -110,7 +110,14 @@ private void handleKeepAliveTimeout() {
log.debug("[{}] Sending ping message", ctx.channel());
}
waitingForPingResponse = true;
- ctx.writeAndFlush(Commands.newPing());
+ ctx.writeAndFlush(Commands.newPing())
+ .addListener(future -> {
+ if (!future.isSuccess()) {
+ log.warn("[{}] Forcing connection to close since cannot send a ping message.",
+ ctx.channel(), future.cause());
+ ctx.close();
Review Comment:
Good. Please ignore this comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari merged pull request #15382: [Broker/Client] Close connection if a ping or pong message cannot be sent
Posted by GitBox <gi...@apache.org>.
lhotari merged PR #15382:
URL: https://github.com/apache/pulsar/pull/15382
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #15382: [Broker/Client] Close connection if a ping message cannot be sent
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15382:
URL: https://github.com/apache/pulsar/pull/15382#issuecomment-1113039333
I added similar handling for the Pong message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org