You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/11/13 13:27:26 UTC
[GitHub] [activemq-artemis] ldebello opened a new pull request #3339: Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
ldebello opened a new pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339
Sending this small fix to resolve the ticket https://issues.apache.org/jira/browse/ARTEMIS-2991
# Implementation
The idea is to remove the HttpAcceptorHandler from the list of handlers when the upgrade is perform
# Test
1) Do you know a good way of testing this? I was thinking using JMSWebSocketConnectionTest to later get the NettyAcceptor but not sure about how to get it, I can check other approaches but I would like to know your opinion before creating the test
2) Also I need to test the the websocket connection keeps opens without the need of that handler.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-728464456
> I don't see any problem with adding getters for testing in this kind of situation. None of these classes is part of a "public" API. Testing is a critical part of development and tests should be able to get all the state they need from a class in order to test it effectively. It's much simpler and straight-forward to just add the getters instead of using something ugly and brittle like reflection. Also, mocking is good for a lot things, but we need an actual integration test here.
I have added the get and I created a small interface for the keep alive handler, let me know if you are ok with the changes or if you think another test should be added
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-728236906
> @ldebello, 2.16.0 has already been released. The release announcement was sent to the ActiveMQ User List. You can find the announcement in the [list archive](http://mail-archives.apache.org/mod_mbox/activemq-users/202011.mbox/%3CCAKF%2BbsrKmN%3D0WbfQgUcbPz5558WiaBPZJe9bHUF%3D2Gh5t%3DbTAg%40mail.gmail.com%3E). The download is available [here](http://activemq.apache.org/components/artemis/download/).
>
> If you really want/need this fix you can always download the tag of 2.16.0, apply this fix, and either build a full release (e.g. using `mvn clean install -Prelease` or just build the jar for the `activemq-server` module and drop that in to your existing environment.
>
> As far as this PR goes you need to squash your commits and then reference ARTEMIS-2991 in the commit message as described in the [hacking guide](https://github.com/apache/activemq-artemis/blob/master/docs/hacking-guide/en/code.md#commitMessageDetails).
Thanks for the response, I have already squash the commits and change the message, thanks for the help
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3339: Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-728135705
@ldebello, 2.16.0 has already been released. The release announcement was sent to the ActiveMQ User List. You can find the announcement in the [list archive](http://mail-archives.apache.org/mod_mbox/activemq-users/202011.mbox/%3CCAKF%2BbsrKmN%3D0WbfQgUcbPz5558WiaBPZJe9bHUF%3D2Gh5t%3DbTAg%40mail.gmail.com%3E). The download is available [here](http://activemq.apache.org/components/artemis/download/).
If you really want/need this fix you can always download the tag of 2.16.0, apply this fix, and either build a full release (e.g. using `mvn clean install -Prelease` or just build the jar for the `activemq-server` module and drop that in to your existing environment.
As far as this PR goes you need to squash your commits and then reference ARTEMIS-2991 in the commit message as described in the [hacking guide](https://github.com/apache/activemq-artemis/blob/master/docs/hacking-guide/en/code.md#commitMessageDetails).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3339: Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-726782285
Using `handlerRemoved` seems like a legitimate fix. Only testing will confirm, though.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-729011909
@jbertram I have done the change, let me know your thoughts
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-729371697
@jbertram I'm out of context on this PR.. if you could merge this one ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-727200962
I have added a test to verify the unregister is called
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-729000616
I don't think the interface is necessary until there is a clear need for a different implementation. I would remove it to simplify the code. In any event, the name of the interface was confusing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3339: Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-726773650
Also, for what it's worth, I don't think the HTTP keep-alive is necessary for the websocket connection. Websockets already have a ping/pong mechanism.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3339: Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-726767632
If you're going to pursue this solution you should probably just implement the `handlerRemoved` method on `org.apache.activemq.artemis.core.remoting.impl.netty.HttpAcceptorHandler`, e.g.:
```java
@Override
public void handlerRemoved(final ChannelHandlerContext ctx) throws Exception {
super.handlerRemoved(ctx);
httpKeepAliveTask.unregisterKeepAliveHandler(this);
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-728295903
> It's common to add getters or other helper methods when writing tests. You'll need to add a method to get the `ProtocolHandler` from the `NettyAcceptor` as well as a method to get the `HttpKeepAliveRunnable` from the `ProtocolHandler` and finally a method to get the `List<HttpAcceptorHandler>` from the `HttpKeepAliveRunnable`.
>
> Once you create/configure/start the embedded server for your test you can use the `org.apache.activemq.artemis.core.server.ActiveMQServer#getRemotingService` method and then `org.apache.activemq.artemis.core.remoting.server.RemotingService#getAcceptor`. Once you have the right `Acceptor` instance you can use the methods you added to get the list of handlers. Your test should create a bunch of websocket connections and make sure the size of the handler list doesn't keep growing.
I am not a very fond of adding getters for testing but if that is ok I will try to go on path, I will include additional tests to validate the fix, thanks for the help
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-727211276
One extra question I saw there is a tag for 2.16.0 not sure if possible to include this for that version or we will need to wait until 2.17.0 or 2.16.x currently we are restarting our broker every two weeks to avoid issues from the leak handlers
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-728310654
I don't see any problem with adding getters for testing in this kind of situation. None of these classes is part of a "public" API. Testing is a critical part of development and tests should be able to get all the state they need from a class in order to test it effectively. It's much simpler and straight-forward to just add the getters instead of using something ugly and brittle like reflection. Also, mocking is good for a lot things, but we need an actual integration test here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-728293307
It's common to add getters or other helper methods when writing tests. You'll need to add a method to get the `ProtocolHandler` from the `NettyAcceptor` as well as a method to get the `HttpKeepAliveRunnable` from the `ProtocolHandler` and finally a method to get the `List<HttpAcceptorHandler>` from the `HttpKeepAliveRunnable`.
Once you create/configure/start the embedded server for your test you can use the `org.apache.activemq.artemis.core.server.ActiveMQServer#getRemotingService` method and then `org.apache.activemq.artemis.core.remoting.server.RemotingService#getAcceptor`. Once you have the right `Acceptor` instance you can use the methods you added to get the list of handlers. Your test should create a bunch of websocket connections and make sure the size of the handler list doesn't keep growing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-728261306
This looks good for the most part, but I think the test is a bit weak. I would like to see an integration test that would fail before your fix and succeed after it which tests the actual number of handlers (i.e. the size of `org.apache.activemq.artemis.core.remoting.impl.netty.HttpKeepAliveRunnable#handlers`). Let me know if you need help implementing this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-729003226
> I don't think the interface is necessary until there is a clear need for a different implementation. I would remove it to simplify the code. In any event, the name of the interface was confusing.
np, I can remove it :). I will do the change, thanks for the help
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-726776209
> Also, for what it's worth, I don't think the HTTP keep-alive is necessary for the websocket connection. Websockets already have a ping/pong mechanism.
Yes I think the same but I need to verify that
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-728979931
@ldebello, was there a specific reason for adding a new interface? It seems unnecessary to me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-726775835
> If you're going to pursue this solution you should probably just implement the `handlerRemoved` method on `org.apache.activemq.artemis.core.remoting.impl.netty.HttpAcceptorHandler`, e.g.:
>
> ```java
> @Override
> public void handlerRemoved(final ChannelHandlerContext ctx) throws Exception {
> super.handlerRemoved(ctx);
> httpKeepAliveTask.unregisterKeepAliveHandler(this);
> }
> ```
Awesome I didn't see that method that would be better. I will change it. Do you think is the correct fix or I need to something better?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-728990814
> @ldebello, was there a specific reason for adding a new interface? It seems unnecessary to me.
I thought it was better, to later try to provide a new implement for the runnable. I saw a PR including JMH tests so I thought about waiting for that PR to be merged to try to change the current implementation providing some tests.
If you think that is not necessary I can change it, to include the getter and return an inmutable list of handlers.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-727205583
Let me know if I am missing something or if you have some comments
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] ldebello commented on pull request #3339: ARTEMIS-2991 Removing HttpAcceptorHandler from HttpKeepAliveRunnable when upgrade to websocket connection
Posted by GitBox <gi...@apache.org>.
ldebello commented on pull request #3339:
URL: https://github.com/apache/activemq-artemis/pull/3339#issuecomment-728266306
> This looks good for the most part, but I think the test is a bit weak. I would like to see an integration test that would fail before your fix and succeed after it which tests the actual number of handlers (i.e. the size of `org.apache.activemq.artemis.core.remoting.impl.netty.HttpKeepAliveRunnable#handlers`). Let me know if you need help implementing this.
I totally agree with you, I tested manually because I didnt find a good way of accesing to the handler list, because the ProtocolHandler which creates the Runnables lives is created by the NettyAcceptor in the constructor. One option could be reflection in each instance until reach the handler seems a little dirty but possible, mocking everything seems kind of hard, what do you think is the best wait to test this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org