You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by clebertsuconic <gi...@git.apache.org> on 2018/06/19 21:44:36 UTC
[GitHub] activemq-artemis pull request #2149: ARTEMIS-1924 Test consumer cleanup afte...
GitHub user clebertsuconic opened a pull request:
https://github.com/apache/activemq-artemis/pull/2149
ARTEMIS-1924 Test consumer cleanup after socket connection reset
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/clebertsuconic/activemq-artemis ARTEMIS-1924
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2149.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 #2149
----
commit e0f28e8471fd1306b4455c285e9561339303127a
Author: Clebert Suconic <cl...@...>
Date: 2018-06-19T21:42:46Z
ARTEMIS-1924 Test consumer cleanup after socket connection reset
----
---
[GitHub] activemq-artemis pull request #2149: ARTEMIS-1924 Test consumer cleanup afte...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2149#discussion_r196726608
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpNoHearbeatsTest.java ---
@@ -80,4 +88,68 @@ public void inspectOpenedResource(Connection connection) {
connection.close();
}
+ private static final String QUEUE_NAME = "queue://testHeartless";
+
+ // This test is validating a scenario where the client will leave with connection reset
+ // This is done by setting soLinger=0 on the socket, which will make the system to issue a connection.reset instead of sending a
+ // disconnect.
+ @Test(timeout = 60000)
+ public void testCloseConsumerOnConnectionReset() throws Exception {
+
+ AmqpClient client = createAmqpClient();
+ assertNotNull(client);
+
+ client.setValidator(new AmqpValidator() {
+
+ @Override
+ public void inspectOpenedResource(Connection connection) {
+ assertEquals("idle timeout was not disabled", 0, connection.getTransport().getRemoteIdleTimeout());
+ }
+ });
+
+ AmqpConnection connection = addConnection(client.connect());
+ assertNotNull(connection);
+
+ connection.getStateInspector().assertValid();
+ AmqpSession session = connection.createSession();
+ AmqpReceiver receiver = session.createReceiver(QUEUE_NAME);
+
+ // This test needs a remote process exiting without closing the socket
+ // with soLinger=0 on the socket so it will issue a connection.reset
+ Process p = SpawnedVMSupport.spawnVM(AmqpNoHearbeatsTest.class.getName(), "testConnectionReset");
--- End diff --
Would using the test name be a better idea here to make it align better?
---
[GitHub] activemq-artemis pull request #2149: ARTEMIS-1924 Test consumer cleanup afte...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2149#discussion_r196726944
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpNoHearbeatsTest.java ---
@@ -80,4 +88,68 @@ public void inspectOpenedResource(Connection connection) {
connection.close();
}
+ private static final String QUEUE_NAME = "queue://testHeartless";
+
+ // This test is validating a scenario where the client will leave with connection reset
+ // This is done by setting soLinger=0 on the socket, which will make the system to issue a connection.reset instead of sending a
+ // disconnect.
+ @Test(timeout = 60000)
+ public void testCloseConsumerOnConnectionReset() throws Exception {
+
+ AmqpClient client = createAmqpClient();
+ assertNotNull(client);
+
+ client.setValidator(new AmqpValidator() {
+
+ @Override
+ public void inspectOpenedResource(Connection connection) {
+ assertEquals("idle timeout was not disabled", 0, connection.getTransport().getRemoteIdleTimeout());
+ }
+ });
+
+ AmqpConnection connection = addConnection(client.connect());
+ assertNotNull(connection);
+
+ connection.getStateInspector().assertValid();
+ AmqpSession session = connection.createSession();
+ AmqpReceiver receiver = session.createReceiver(QUEUE_NAME);
+
+ // This test needs a remote process exiting without closing the socket
+ // with soLinger=0 on the socket so it will issue a connection.reset
+ Process p = SpawnedVMSupport.spawnVM(AmqpNoHearbeatsTest.class.getName(), "testConnectionReset");
+ Assert.assertEquals(33, p.waitFor());
+
+ AmqpSender sender = session.createSender(QUEUE_NAME);
+
+ for (int i = 0; i < 10; i++) {
+ AmqpMessage msg = new AmqpMessage();
+ msg.setBytes(new byte[] {0});
+ sender.send(msg);
+ }
+
+ receiver.flow(20);
+
+ for (int i = 0; i < 10; i++) {
+ AmqpMessage msg = receiver.receive(1, TimeUnit.SECONDS);
+ Assert.assertNotNull(msg);
+ msg.accept();
+ }
+ }
+
+ public static void main(String[] arg) {
+ if (arg.length > 0 && arg[0].equals("testConnectionReset")) {
+ try {
+ AmqpClient client = new AmqpClient(new URI("tcp://127.0.0.1:5672?transport.soLinger=0"), null, null);
+ AmqpConnection connection = client.connect();
+ AmqpSession session = connection.createSession();
+ AmqpReceiver receiver = session.createReceiver(QUEUE_NAME);
--- End diff --
Linked with above comments, using the actual test name as the argument (instead of the differerent "testConnectionReset" literal) would mean this queue name constant wouldnt be needed and the argument could be used directly.
---
[GitHub] activemq-artemis pull request #2149: ARTEMIS-1924 Test consumer cleanup afte...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2149#discussion_r196752073
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpNoHearbeatsTest.java ---
@@ -80,4 +88,68 @@ public void inspectOpenedResource(Connection connection) {
connection.close();
}
+ private static final String QUEUE_NAME = "queue://testHeartless";
--- End diff --
I made a mistake. I created a static property because this needs a Spawned VM. Getting it from the static property seemed easier.
Do you really care about this? if so we can change the test.
---
[GitHub] activemq-artemis pull request #2149: ARTEMIS-1924 Test consumer cleanup afte...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2149#discussion_r196724509
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpNoHearbeatsTest.java ---
@@ -80,4 +88,68 @@ public void inspectOpenedResource(Connection connection) {
connection.close();
}
+ private static final String QUEUE_NAME = "queue://testHeartless";
--- End diff --
Why does a test named "testCloseConsumerOnConnectionReset" use a queue named "queue://testHeartless", when "testHeartless" is the name of a different test in the class? Does it need a "queue://" prefix?
I'd just use getTestName() inside the test get a helpful value.
---
[GitHub] activemq-artemis pull request #2149: ARTEMIS-1924 Test consumer cleanup afte...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2149#discussion_r196751676
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpNoHearbeatsTest.java ---
@@ -80,4 +88,68 @@ public void inspectOpenedResource(Connection connection) {
connection.close();
}
+ private static final String QUEUE_NAME = "queue://testHeartless";
+
+ // This test is validating a scenario where the client will leave with connection reset
+ // This is done by setting soLinger=0 on the socket, which will make the system to issue a connection.reset instead of sending a
+ // disconnect.
+ @Test(timeout = 60000)
+ public void testCloseConsumerOnConnectionReset() throws Exception {
--- End diff --
The two are related... you should be able to cleanup consumer, disable heartbeats and call disconnect on windows. ARTEMIS-1927 was found after I disabled heart beats and killed a consumer on windows which issued a connection reset.
---
[GitHub] activemq-artemis pull request #2149: ARTEMIS-1924 Test consumer cleanup afte...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2149#discussion_r196814636
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpNoHearbeatsTest.java ---
@@ -80,4 +88,68 @@ public void inspectOpenedResource(Connection connection) {
connection.close();
}
+ private static final String QUEUE_NAME = "queue://testHeartless";
+
+ // This test is validating a scenario where the client will leave with connection reset
+ // This is done by setting soLinger=0 on the socket, which will make the system to issue a connection.reset instead of sending a
+ // disconnect.
+ @Test(timeout = 60000)
+ public void testCloseConsumerOnConnectionReset() throws Exception {
--- End diff --
To me it seems that the broker behaviour should essentially be the same here regardless whether idle-timeout is enabled or not, i.e a connection explicitly goes away, the broker cleans up a consumer on it, and its not really testing idle-timeout behaviour or the lack of it, making it feels a bit odd it being in here.
---
[GitHub] activemq-artemis pull request #2149: ARTEMIS-1924 Test consumer cleanup afte...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2149#discussion_r196726336
--- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AmqpNoHearbeatsTest.java ---
@@ -80,4 +88,68 @@ public void inspectOpenedResource(Connection connection) {
connection.close();
}
+ private static final String QUEUE_NAME = "queue://testHeartless";
+
+ // This test is validating a scenario where the client will leave with connection reset
+ // This is done by setting soLinger=0 on the socket, which will make the system to issue a connection.reset instead of sending a
+ // disconnect.
+ @Test(timeout = 60000)
+ public void testCloseConsumerOnConnectionReset() throws Exception {
--- End diff --
I dont really understand why this test is in the AmqpNoHearbeatsTest class, it feels like it should operate about the same regardless whether AMQP idle-timeout handling is enabled or not. Its definitely not where I would go looking for such a test.
It also seems far more appropriate to testing the changes made in #2139 for ARTEMIS-1927 (without test).
---
[GitHub] activemq-artemis pull request #2149: ARTEMIS-1924 Test consumer cleanup afte...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2149
---