Hadoop Common
  1. Hadoop Common
  2. HADOOP-7695

RPC.stopProxy can throw unintended exception while logging error

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.24.0
    • Fix Version/s: 2.0.0-alpha
    • Component/s: ipc
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      RPC.stopProxy includes the following lines in case of error:

      LOG.error("Could not get invocation handler " + invocationHandler +
                " for proxy " + proxy + ", or invocation handler is not closeable.");
      

      Trouble is, the proxy object is usually backed by WritableRpcEngine, which will fail in the event toString is called on one of its proxy objects. See HADOOP-7694 for more details on that issue. Until that's addressed, we might as well change the log message in RPC.stopProxy to not call toString() on proxy.

      1. HADOOP-7695.patch
        0.7 kB
        Aaron T. Myers

        Issue Links

          Activity

          Hide
          Uma Maheswara Rao G added a comment -

          In that case can i put the same patch (HADOOP-7694) here?

          Show
          Uma Maheswara Rao G added a comment - In that case can i put the same patch ( HADOOP-7694 ) here?
          Hide
          Aaron T. Myers added a comment -

          In that case can i put the same patch (HADOOP-7694) here?

          Note my comment in HADOOP-7607 where I said:

          The patch Uma supplied won't quite work as-is, since the proxy and invocationHandler objects can't be assumed to be non-null, but it's close.

          I'd prefer to take care of this JIRA, if you don't mind, Uma.

          Show
          Aaron T. Myers added a comment - In that case can i put the same patch ( HADOOP-7694 ) here? Note my comment in HADOOP-7607 where I said: The patch Uma supplied won't quite work as-is, since the proxy and invocationHandler objects can't be assumed to be non-null, but it's close. I'd prefer to take care of this JIRA, if you don't mind, Uma.
          Hide
          Uma Maheswara Rao G added a comment -

          Sorry, i put the patch in HADOOP-7607

          Show
          Uma Maheswara Rao G added a comment - Sorry, i put the patch in HADOOP-7607
          Hide
          Uma Maheswara Rao G added a comment -

          The patch Uma supplied won't quite work as-is, since the proxy and invocationHandler objects can't be assumed to be non-null, but it's close

          Are you ok with the same patch?
          (or) Expecting some change?
          Can you please clarify.

          Show
          Uma Maheswara Rao G added a comment - The patch Uma supplied won't quite work as-is, since the proxy and invocationHandler objects can't be assumed to be non-null, but it's close Are you ok with the same patch? (or) Expecting some change? Can you please clarify.
          Hide
          Aaron T. Myers added a comment -

          Are you ok with the same patch? (or) Expecting some change?

          No, the same patch won't work. Note that the else clause where the log message is printed could be reached if the proxy or invocationHandler objects are null, so it's not safe to call .getClass() on the objects, as is done in your patch.

          If you feel strongly, I don't mind reassigning this issue to you.

          Show
          Aaron T. Myers added a comment - Are you ok with the same patch? (or) Expecting some change? No, the same patch won't work. Note that the else clause where the log message is printed could be reached if the proxy or invocationHandler objects are null , so it's not safe to call .getClass() on the objects, as is done in your patch. If you feel strongly, I don't mind reassigning this issue to you.
          Hide
          Uma Maheswara Rao G added a comment -

          Hi Aaron,
          Thanks for the clarification, I got your point.

          Is this below proposed change fine?
          Since this is not positive flow, i just handled with conditional statements.

          LOG.error("Could not get invocation handler " + invocationHandler == null ? invocationHandler
                        : invocationHandler.getClass() + " for proxy " + proxy == null ? proxy
                            : proxy.getClass()
                                + ", or invocation handler is not closeable.");
          

          Thanks
          Uma

          Show
          Uma Maheswara Rao G added a comment - Hi Aaron, Thanks for the clarification, I got your point. Is this below proposed change fine? Since this is not positive flow, i just handled with conditional statements. LOG.error( "Could not get invocation handler " + invocationHandler == null ? invocationHandler : invocationHandler.getClass() + " for proxy " + proxy == null ? proxy : proxy.getClass() + ", or invocation handler is not closeable." ); Thanks Uma
          Hide
          Aaron T. Myers added a comment -

          Here's a patch which addresses the issue.

          Uma, there's not much reason to check that invocationHandler is null, since that object is likely not in fact a proxy object, so it should be safe to call toString() on it. I also prefer to explicitly include null in the string, instead of proxy which happens to be null, to make things more clear.

          Does this look OK to you?

          Show
          Aaron T. Myers added a comment - Here's a patch which addresses the issue. Uma, there's not much reason to check that invocationHandler is null, since that object is likely not in fact a proxy object, so it should be safe to call toString() on it. I also prefer to explicitly include null in the string, instead of proxy which happens to be null , to make things more clear. Does this look OK to you?
          Hide
          Uma Maheswara Rao G added a comment -

          Make sense, Patch looks good to e.
          +1

          Show
          Uma Maheswara Rao G added a comment - Make sense, Patch looks good to e. +1
          Hide
          Uma Maheswara Rao G added a comment -

          One small nit,
          i don't prefer printing invocation object reference numbers along with class names in logs. org.apache.hadoop.io.retry.RetryInvocationHandler@900e24. Here intention is just to print the class name of RetryInvocationHandler to know which handler it is.
          This comment is not major, you can take decision.

          Thanks
          Uma

          Show
          Uma Maheswara Rao G added a comment - One small nit, i don't prefer printing invocation object reference numbers along with class names in logs. org.apache.hadoop.io.retry.RetryInvocationHandler@900e24. Here intention is just to print the class name of RetryInvocationHandler to know which handler it is. This comment is not major, you can take decision. Thanks Uma
          Hide
          Aaron T. Myers added a comment -

          Thanks for the review, Uma. I'd prefer to stick with toString() where possible, to allow for implementations of InvocationHandler to provide their own implementation of toString() that makes sense for them.

          Show
          Aaron T. Myers added a comment - Thanks for the review, Uma. I'd prefer to stick with toString() where possible, to allow for implementations of InvocationHandler to provide their own implementation of toString() that makes sense for them.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12496958/HADOOP-7695.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in .

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/244//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/244//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12496958/HADOOP-7695.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/244//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/244//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          No tests included, as this is just changing a log message.

          Show
          Aaron T. Myers added a comment - No tests included, as this is just changing a log message.
          Hide
          Todd Lipcon added a comment -

          +1

          Show
          Todd Lipcon added a comment - +1
          Hide
          Aaron T. Myers added a comment -

          Thanks a lot for the review, Todd. I've just committed this.

          Thanks also to Uma for noticing this issue.

          Show
          Aaron T. Myers added a comment - Thanks a lot for the review, Todd. I've just committed this. Thanks also to Uma for noticing this issue.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #1025 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1025/)
          HADOOP-7695. RPC.stopProxy can throw unintended exception while logging error (atm)

          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179512
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #1025 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1025/ ) HADOOP-7695 . RPC.stopProxy can throw unintended exception while logging error (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179512 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #1103 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1103/)
          HADOOP-7695. RPC.stopProxy can throw unintended exception while logging error (atm)

          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179512
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #1103 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1103/ ) HADOOP-7695 . RPC.stopProxy can throw unintended exception while logging error (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179512 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #1041 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1041/)
          HADOOP-7695. RPC.stopProxy can throw unintended exception while logging error (atm)

          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179512
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #1041 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1041/ ) HADOOP-7695 . RPC.stopProxy can throw unintended exception while logging error (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179512 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #822 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/822/)
          HADOOP-7695. RPC.stopProxy can throw unintended exception while logging error (atm)

          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179512
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #822 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/822/ ) HADOOP-7695 . RPC.stopProxy can throw unintended exception while logging error (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179512 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #852 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/852/)
          HADOOP-7695. RPC.stopProxy can throw unintended exception while logging error (atm)

          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179512
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #852 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/852/ ) HADOOP-7695 . RPC.stopProxy can throw unintended exception while logging error (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179512 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have merged this to 0.23.

          Show
          Tsz Wo Nicholas Sze added a comment - I have merged this to 0.23.

            People

            • Assignee:
              Aaron T. Myers
              Reporter:
              Aaron T. Myers
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development