[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/activemq-artemis/pull/1534


---

[GitHub] activemq-artemis pull request #1534: ARTEMIS-1420 limit non-ssl connection h...

[jbertram]

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 pull request #1534: ARTEMIS-1420 limit non-ssl connection h...

[jbertram]

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...

[jbertram]

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...

[jbertram]

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 pull request #1534: ARTEMIS-1420 limit non-ssl connection h...

[jbertram]

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...

[jbertram]

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 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 pull request #1534: ARTEMIS-1420 limit non-ssl connection h...

[stanlyDoge]

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 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...

[michaelandrepearce]

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 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...

[michaelandrepearce]

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...

[michaelandrepearce]

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 pull request #1534: ARTEMIS-1420 limit non-ssl connection h...

[michaelandrepearce]

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 pull request #1534: ARTEMIS-1420 limit non-ssl connection h...

[michaelandrepearce]

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...

[michaelandrepearce]

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...

[michaelandrepearce]

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...

[stanlyDoge]

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 
Date:   2017-09-13T14:50:42Z

ARTEMIS-1420




---