ARTEMIS-1924 Test consumer cleanup after socket connection reset#2149
Conversation
e0f28e8 to
69d9b60
Compare
| connection.close(); | ||
| } | ||
|
|
||
| private static final String QUEUE_NAME = "queue://testHeartless"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // 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 { |
There was a problem hiding this comment.
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 https://issues.apache.org/jira/browse/ARTEMIS-1927 (without test).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| // 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"); |
There was a problem hiding this comment.
Would using the test name be a better idea here to make it align better?
| 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); |
There was a problem hiding this comment.
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.
3 participants
No description provided.