You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by stanlyDoge <gi...@git.apache.org> on 2017/09/13 14:52:40 UTC
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
GitHub user stanlyDoge opened a pull request:
https://github.com/apache/activemq-artemis/pull/1534
ARTEMIS-1420 limit non-ssl connection hangs up exceeding client(s)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/stanlyDoge/activemq-artemis-1 ARTEMIS-1420
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1534.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1534
----
commit 518e5c5491aa7261b6864b81617434aadd6e34c1
Author: Stanislav Knot <sk...@redhat.com>
Date: 2017-09-13T14:50:42Z
ARTEMIS-1420
----
---
[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/activemq-artemis/pull/1534
> "Connection readTimeout has occurred.",
NETTY_READ_TIMEOUT = "read-timeout-seconds";
private int readTimeout;
Would a more descriptive name be better here for the configuration etc, something that conveys its a one-time protocol header timeout at connection startup? Using simply 'read timeout' seems like it could be confused with being related to other more general ongoing TCP read timeout handling and heartbeating etc, whereas it actually wont have effect beyond the first 8 bytes. Maybe 'protocol-header-timeout' or something like that?
The test was added in the amqp test package as opposed to somewhere more general, which would seem nicer given it is entirely protocol-agnostic behaviour under test. Overlooking that though it was added to "AmqpSendReceiveTest" which seems odd given it obviously doesn't establish an AMQP connection and send or receive anything. Adding it there also means the test had to start->stop->reconfigure->restart the broker to perform the checking. Adding a new test class would be nicer, preferably outside the amqp package, but even if shoehorning into an existing amqp test there are perhaps better options like AmqpProtocolHeaderHandlingTest where it could be taken as a 'doesnt send header' test.
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r138714254
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java ---
@@ -206,6 +207,8 @@
final AtomicBoolean warningPrinted = new AtomicBoolean(false);
+ private static int communicationTimeout = 30; //seconds
--- End diff --
Also shouldn't be static, as you may want to configure different acceptors to different configs
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r145710225
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java ---
@@ -38,6 +39,7 @@
import io.netty.handler.codec.http.HttpResponseEncoder;
import org.apache.activemq.artemis.api.core.client.ActiveMQClient;
import org.apache.activemq.artemis.core.buffers.impl.ChannelBufferWrapper;
+import org.apache.activemq.artemis.core.client.ActiveMQClientLogger;
--- End diff --
Since the timeout is being identified by the server the ActiveMQServerLogger should be used here, not the client logger.
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r138720142
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java ---
@@ -206,6 +207,8 @@
final AtomicBoolean warningPrinted = new AtomicBoolean(false);
+ private static int communicationTimeout = 30; //seconds
--- End diff --
I have done a very quick (not 100% complete) example of the way to better config this and how to have default (with disabled), and also how to add specific logger for the other review comment i left.
If you want to look at:
https://github.com/michaelandrepearce/activemq-artemis/commit/4e2026c0fce5a75dcb9551fc4237d3cba9abfb23
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r138716684
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java ---
@@ -843,4 +847,12 @@ private Throwable getRootCause(Throwable throwable) {
return (list.size() < 2 ? throwable : list.get(list.size() - 1));
}
}
+
+ public static int getCommunicationTimeout() {
--- End diff --
as noted about should be configured by configuration per acceptor instance, these then are redundant.
---
[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1534
@stanlyDoge I'm happy with this version of the implementation. The only concern I see is that by default the read timeout is switched off. This should be some sensible value, significantly less than the default TCP TTL. I would say ~10 seconds is sufficient.
---
[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1534
@mbukosky Hi, could you open a JIRA here: https://issues.apache.org/jira/projects/ARTEMIS. You can add a description of the issue and potential solution. Also a PR with the fix would be very much appreciated. Thanks
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by stanlyDoge <gi...@git.apache.org>.
Github user stanlyDoge commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r139663458
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpClientTestSupport.java ---
@@ -185,6 +187,7 @@ protected TransportConfiguration addAcceptorConfiguration(ActiveMQServer server,
HashMap<String, Object> params = new HashMap<>();
params.put(TransportConstants.PORT_PROP_NAME, String.valueOf(port));
params.put(TransportConstants.PROTOCOLS_PROP_NAME, getConfiguredProtocols());
+ params.put(TransportConstants.NETTY_READ_TIMEOUT, readTimeout);
--- End diff --
Actually it is 3 seconds. It is just typo in property string. I've tried to make test as you say. Check it in new commit. And thanks for reviewing! :)
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1534
---
[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1534
@stanlyDoge, can you modify this in the following ways:
- Remove the ReadTimeoutHandler from the pipeline when the handshake is complete so it doesn't interfere with the other connection monitoring logic.
- Change the test so it only tests the handshake bit.
---
[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1534
@mbukosky, please note that a bug regarding this change was just resolved. See #1750.
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r145710480
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java ---
@@ -99,9 +101,28 @@ public ProtocolManager getProtocol(String name) {
private final boolean httpEnabled;
+ private ScheduledFuture timeoutFuture;
+
+ private int handshakeTimeout;
+
+
ProtocolDecoder(boolean http, boolean httpEnabled) {
this.http = http;
this.httpEnabled = httpEnabled;
+ this.handshakeTimeout = ConfigurationHelper.getIntProperty(TransportConstants.NETTY_HANDSHAKE_TIMEOUT, TransportConstants.DEFAULT_NETTY_HANDSHAKE_TIMEOUT, nettyAcceptor.getConfiguration());
+ }
+
+ @Override
+ public void channelActive(ChannelHandlerContext ctx) throws Exception {
+ if (handshakeTimeout > 0) {
+ timeoutFuture = scheduledThreadPool.schedule(new Runnable() {
+ @Override
--- End diff --
This could be a bit more readable using a lambda. Just a thought.
---
[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1534
Can you elaborate on the use case where this is required? The broker already has functionality to close dead connections. By implementing this feature you are hard-coding a 30 timeout for *all* clients regardless of how they've configured their heart-beats.
---
[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...
Posted by mbukosky <gi...@git.apache.org>.
Github user mbukosky commented on the issue:
https://github.com/apache/activemq-artemis/pull/1534
I found a bug with this change. I also have a possible solution. How should I got about requesting a fix? Thanks
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r138716284
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/ActiveMQChannelHandler.java ---
@@ -89,6 +90,11 @@ public void exceptionCaught(final ChannelHandlerContext ctx, final Throwable cau
if (!active) {
return;
}
+
+ if (cause instanceof ReadTimeoutException) {
+ ActiveMQClientLogger.LOGGER.error("Connection without communication timeout has expired.");
--- End diff --
Should probably have a specific error code defined in ActiveMQClientLogger and use that.
---
[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...
Posted by stanlyDoge <gi...@git.apache.org>.
Github user stanlyDoge commented on the issue:
https://github.com/apache/activemq-artemis/pull/1534
@mtaylor Thanks, changed.
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r139573407
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpClientTestSupport.java ---
@@ -185,6 +187,7 @@ protected TransportConfiguration addAcceptorConfiguration(ActiveMQServer server,
HashMap<String, Object> params = new HashMap<>();
params.put(TransportConstants.PORT_PROP_NAME, String.valueOf(port));
params.put(TransportConstants.PROTOCOLS_PROP_NAME, getConfiguredProtocols());
+ params.put(TransportConstants.NETTY_READ_TIMEOUT, readTimeout);
--- End diff --
3 ms seems very small even for test case, also test should test without setting this that error does occur. Eg assert the problem exists without setting.
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r145710804
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/ProtocolHandler.java ---
@@ -99,9 +101,28 @@ public ProtocolManager getProtocol(String name) {
private final boolean httpEnabled;
+ private ScheduledFuture timeoutFuture;
+
+ private int handshakeTimeout;
+
+
ProtocolDecoder(boolean http, boolean httpEnabled) {
this.http = http;
this.httpEnabled = httpEnabled;
+ this.handshakeTimeout = ConfigurationHelper.getIntProperty(TransportConstants.NETTY_HANDSHAKE_TIMEOUT, TransportConstants.DEFAULT_NETTY_HANDSHAKE_TIMEOUT, nettyAcceptor.getConfiguration());
+ }
+
+ @Override
+ public void channelActive(ChannelHandlerContext ctx) throws Exception {
+ if (handshakeTimeout > 0) {
+ timeoutFuture = scheduledThreadPool.schedule(new Runnable() {
+ @Override
+ public void run() {
+ ActiveMQClientLogger.LOGGER.readTimeout();
--- End diff --
The method should probably be renamed something like handshakeTimeout() in addition to being moved to the ActiveMQServerLogger.
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r138713895
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java ---
@@ -206,6 +207,8 @@
final AtomicBoolean warningPrinted = new AtomicBoolean(false);
+ private static int communicationTimeout = 30; //seconds
--- End diff --
This should be named read time out similar to netty naming
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r146324088
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/TransportConstants.java ---
@@ -255,6 +255,10 @@
public static final int DEFAULT_STOMP_MAX_FRAME_PAYLOAD_LENGTH = 65536;
+ public static final String NETTY_HANDSHAKE_TIMEOUT = "protocol-header-timeout";
--- End diff --
Names here should be consistent to avoid confusion between the code/config. Maybe something like:
HANDSHAKE_TIMEOUT = "handshake-timeout"
---
[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...
Posted by stanlyDoge <gi...@git.apache.org>.
Github user stanlyDoge commented on the issue:
https://github.com/apache/activemq-artemis/pull/1534
I've already pushed removing ReadTimeoutHandler [here](https://github.com/apache/activemq-artemis/pull/1534/commits/10c9495370d3bcaf2644296e1240973441c7213e#diff-7b6326b533c4bbb0db4e315e781d8cbcR734) but i am not sure if it is enough. Also i have to adapt test as you say.
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r145711210
--- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/ActiveMQClientLogger.java ---
@@ -541,4 +541,9 @@
@Message(id = 214033, value = "Cannot resolve host ",
format = Message.Format.MESSAGE_FORMAT)
void unableToResolveHost(@Cause UnknownHostException e);
+
+ @LogMessage(level = Logger.Level.ERROR)
+ @Message(id = 214034, value = "Timeout while handshaking has occurred.",
--- End diff --
It would be useful to know the actual length of the timeout which elapsed.
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r138714062
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java ---
@@ -206,6 +207,8 @@
final AtomicBoolean warningPrinted = new AtomicBoolean(false);
+ private static int communicationTimeout = 30; //seconds
--- End diff --
This also should be config based with defaults declared as per how other bits are configured and defaulted for netty
---
[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1534#discussion_r141371305
--- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java ---
@@ -729,6 +731,7 @@ public NettyServerConnection createConnection(final ChannelHandlerContext ctx,
public void operationComplete(final io.netty.util.concurrent.Future<Channel> future) throws Exception {
if (future.isSuccess()) {
active = true;
+ ctx.channel().pipeline().remove("readTimeoutHandler");
--- End diff --
This is only valid for SSL connections. The "readTimeoutHandler" should be removed for non-SSL connections as well.
---
[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...
Posted by mbukosky <gi...@git.apache.org>.
Github user mbukosky commented on the issue:
https://github.com/apache/activemq-artemis/pull/1534
@jbertram perfect. Thanks for the help. I'll wait until the 2.5.0 release.
---