activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jimbogithub <...@git.apache.org>
Subject [GitHub] activemq-artemis pull request:
Date Thu, 31 Dec 2015 21:14:41 GMT
Github user jimbogithub commented on the pull request:

    https://github.com/apache/activemq-artemis/commit/b1b4bb8a32a9bb9c6d951f3d0fdd3626944319f9#commitcomment-15223944
  
    In artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnection.java:
    In artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnection.java
on line 105:
    It may take a while to come up with a minimal test.  I'm observing this in an existing
app on WildFly 10 and it could take some time to produce a simpler standalone test.  I'll
have a go at producing a test if it's really required, but would it be possible for you to
do a code inspection first?
    
    The only objects that are growing are the ConcurrentLinkedDeque$Node instances as shown
in the first image.  There are no application messages or other JMS components growing and
all messages are being delivered as expected.  The ConcurrentLinkedDeque$Node instances are
definitely related to the NettyServerConnection as shown in the second image.  This problem
only arose after upgrading to a WildFly version that incorporates this change.
    
    Are you certain that...
    
    public boolean isWritable(ReadyListener callback) {
        synchronized (readyListeners) {
            readyListeners.push(callback);
            return ready;
        }
     }
    
    ... shouldn't be ...
    
    public boolean isWritable(ReadyListener callback) {
        synchronized (readyListeners) {
            if (!ready) {
                readyListeners.push(callback);
            }
            return ready;
        }
     }
    
    That would be consistent with the comment on readyListeners.  Otherwise any callers that
continue to call this method after the first time that it returns `true` will trigger this
leak.  There are two such calls I have annotated on this review.
    
    Also note that the readyListeners can be a simple LinkedList as your synchronized blocks
mean that the concurrent implementation is now redundant.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message