I have a case where a thread can finish in 3 ways:
when someCondition is not satisfied;
when some error happens in the connection call (XmppException, NoResponseException or NotConnectedException);
when I interrupt the thread (thread.interrupt()).
See the code.
while (someCondition()) {
** // create iq packet**
** // …**
** try {**
** result = connection.createPacketCollectorAndSend(iq).nextResultOrThrow();**
** // process result**
** // …**
** // check if the thread was interrupted**
** if (Thread.interrupted()) {**
** throw new InterruptedException();**
** }**
** } catch (Exception e) {**
** // cancel the task**
** return;**
** }**
}
Sometimes my code doesn’t know that the thread was interrupted (case 3): if the PacketCollector is waiting for the queue.pool() [see PacketCollector.nextResult(timeout)], the InterruptedException is caught by the loop in nextResult() and my thread.interrupt() call is ignored. IOW, my thread doesn’t finish as I desired.
I think that if the PacketCollector calls an interruptible method (resultQueue.poll), it should throw the InterruptException so that the user can handle it. Furthermore, it should have methods called as “nextResultUninterruptibly” for cases where the user only cares with the timeout (NoResponseException).
Sometimes my code doesn’t know that the thread was interrupted (case 3): if the PacketCollector is waiting for the queue.pool() [see PacketCollector.nextResult(timeout)], the InterruptedException is caught by the loop in nextResult() and my thread.interrupt() call is ignored. IOW, my thread doesn’t finish as I desired.
Yes, the InterruptedException is logged instead of thrown, which is bad practice.
Throwing it would mean an API change, though.
Another possibility is to do Thread.currentThread().interrupt() in the catch clause.
I don’t think nextResultUninterruptibly is needed. If the user does not care he can just ignore the exception.
I have a case where a thread can finish in 3 ways:
when I interrupt the thread (thread.interrupt()).
This assumption is wrong, there is at least a forth way the thread can finish: When something else interrupts the thread. Spurious interrupts are real and allowed the JLS. If your code uses interrupt for control flow, you always have to check a second condition to determine the interrupt cause.
Yes, the InterruptedException is logged instead of thrown, which is bad practice.
I beg to differ, at least in this case. The API was designed to accommodate the use cases most of most users: Receive a reply or await the timeout. It therefore guarantees that only one of two things will make the method return: A packet arrives or the collector has waited the whole timeout value. Exposing the InterruptedException to the user would also tempt those to write wrong code, by assuming that the InterruptedException can only be caused by them calling interrupt().
If you have a third option that could happen while using an IQ, then you should consider using Smack’s async API.
I don’t think nextResultUninterruptibly is needed. If the user does not care he can just ignore the exception.
Which would also mean adding InterruptedException to every method in Smack and user code that calls the method, or wrapping the checked exception into a unchecked one. Those are not good solutions imo.
@andrelab Consider using Smack’s async API (sendIqWithResponseCallback() and Co) here.
So I think the loop should have another test in the loop, in order I can stop before the timeout. It is hard to be generic in this test, but the connection state should be considered.
LOGGER.log(Level.FINE, “nextResult was interrupted”, e);
}
}
I don’t think it is practical the “!cancelled” test. But the isConnected() is. If I close the connection, I don’t need to wait for whole the timeout (that might be too long sometimes).
So I think the loop should have another test in the loop, in order I can stop before the timeout. It is hard to be generic in this test, but the connection state should be considered.
How is this related to your initial issue?
I don’t see a big advantage adding the connection state to the waiting condition.
Suppose that the timeout is defined to 30 seconds.
There is no way to cancel the nextResult(). I have to wait ~30 seconds to the call returns. Even if the connection is closed.
I my particular case, I want to interrupt the thread when the connection is closedOnError. But the ReconnectionManager starts another connection while the old thread is still waiting for nextResult().
PS: I could use the sendIqWithResponseCallback() call, but the nextResultOrThrow is used internally by Smack in a lot of cases.
Better than test if the connection is connected in the while clause is to check the connection and throw an NotConnectedException.
while (res == null && remainingWait > 0) {
** if (!connection.isConnected()) {**
** throw new NotConnectedException();**
** }**
try {
res = § resultQueue.poll(remainingWait, TimeUnit.MILLISECONDS);