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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
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 <[hidden email]>
Date:   2017-09-13T14:50:42Z

    ARTEMIS-1420

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...

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


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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



---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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! :)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.  


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user stanlyDoge commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1534
 
    @mtaylor Thanks, changed.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1534: ARTEMIS-1420 limit non-ssl connection hangs up...

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.


---
Reply | Threaded
Open this post in threaded view
|

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

clebertsuconic-3
In reply to this post by clebertsuconic-3
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.


---
12