Review request: 8049303: Transient network problems cause JMX thread to fail silenty

[Daniel Fuchs]

Hi,

Thanks for the detailed explanations.

The fact that the server doesn't store any client state
and can arbitrarily close the connection, leaving it to
the client to reestablish the connection, makes all this
code quite tricky and hard to follow.

I believe what you propose - making sure that the
notification thread doesn't stop without closing the
connection, is the right thing to do.

I wonder however if the code that closes the connection
should better be moved to ClientNotifForwarder::fetchNotifs?

ClientNotifForwarder::fetchNotifs has the following statement:

601         if (!shouldStop()) {
602             logger.error("NotifFetcher-run",
603                          "Failed to fetch notification, " +
604                          "stopping thread. Error is: " + ioe, ioe);
605             logger.debug("NotifFetcher-run",ioe);
607         }

Then it proceeds to return null, which causes the thread to die.

It looks as if that's the place where the connection should ideally
be closed if it is not already closed, because it would ensure
that the thread never dies silently.

Otherwise I'd suggest improving the comment below:

1369                             // JDK-8049303
1370                             // possible again transient or a
1371                             // non-deserialization exception, not 
know how
1372                             // to treat, close the connection

May I suggest something like:

// JDK-8049303
// We received an IOException - but the communicatorAdmin
// did not close the connection - possibly because the
// the original exception was raised by a transient network
// problem?
// We already know that this exception is not due to a deserialization
// issue as we already took care of that before involving the
// communicatorAdmin. Moreover - we already made one retry attempt
// at fetching the next batch of notifications - and the
// problem persisted.
// Since trying again doesn't seem to solve the issue, we will now
// close the connection. Doing otherwise might cause the
// NotifFetcher thread to die silently.

best regards,

-- daniel

On 9/10/14 5:42 PM, shanliang wrote:
> Hi,
>
> The issue could happen like this:
> 1) RMIConnector.RMINotifClient.fetchNotifs got an IOException
> 2) communicatorAdmin.gotIOException(ioe) was called to check the
> connection, it did not close the connection because the connection was
> now OK.
> 3) RMIConnector.RMINotifClient.fetchNotifs analyzed the original
> exception and found it was not a dersialization exception, it re-threw
> the original IOException
> 4) the caller ClientNotifForwarder did not know how to treat this
> exception, decided to end silently.
>
> The fix is to modify RMIConnector.RMINotifClient.fetchNotifs:
>
> if the fetchNotifs request gets an IOException, we examine the chain of
> exceptions to determine whether this is a deserialization issue. If so -
> we propagate the appropriate exception to the caller, who will then
> proceed with fetching notifications one by one, otherwise we call
> communicatorAdmin.gotIOException(ioe), there are 2 kinds of response:
>     1) the call returns OK, means the connection is re-established, we
> re-call the fetchNotifs;
>     2) the call throws IOException, we check the connection status:
>         2-1) "terminated", that means the connection is closed, we
> re-throw the original IOException, the caller will end silently.
>         2-2) not "terminated", we add a flag "retried" for this
> situation, if the flag is false, we set the flag to true and re-do the
> fetchNotifs request, this is useful for a transient network problem,
> otherwise we close the connection and re-throw the original IOException,
> it is here we fix the bug.
>
> We do not modify communicatorAdmin.gotIOException(ioe), it is called too
> by all other remote requests.
>
> It is not easy to have a test reproducing the bug.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8049303
> webrev: http://cr.openjdk.java.net/~sjiang/JDK-8049303/00/
> <http://cr.openjdk.java.net/%7Esjiang/JDK-8049303/00/>
>
> Thanks,
> Shanliang