Harmony
  1. Harmony
  2. HARMONY-1720

Classlib test org.apache.harmony.luni.tests.java.lang.ThreadTest.test_stop_subtest0 fails

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: DRLVM
    • Labels:
      None

      Description

      The test fails with the following assertion:

      test_stop_subtest0(org.apache.harmony.luni.tests.java.lang.ThreadTest)junit.fework.AssertionFailedError: checkAccess called

      The test does not expect that security manager's checkAccess() method is called when stopping a new thread which is not running yet.
      I think it is a compatibility issue.
      The spec for Thread.stop() says:
      If there is a security manager installed, its checkAccess method is called with this as its argument.

      RI does not call this method for a thread which does not run, and this seems reasonable.
      I suggest that drlvm would do the same.

      NOTE: to reproduce the bug you should comment out ThreadTest.test_suspend() which hangs now. This issue is expected to be fixed with H-1519.

      1. H-1720_combined.patch
        2 kB
        Elena Semukhina
      2. H-1720.patch
        2 kB
        Alexey Varlamov
      3. Harmony-1720_Thread.patch
        0.5 kB
        Elena Semukhina
      4. ThreadStop1.java
        0.9 kB
        Elena Semukhina
      5. ThreadStopThrowable.java
        1 kB
        Elena Semukhina

        Issue Links

          Activity

          Hide
          Geir Magnusson Jr added a comment -

          HARMONY-1519 was applied yesterday, and as far as I can tell, only affected windows.

          Can you comment how this changes things?

          Show
          Geir Magnusson Jr added a comment - HARMONY-1519 was applied yesterday, and as far as I can tell, only affected windows. Can you comment how this changes things?
          Hide
          Elena Semukhina added a comment -

          This is an old issue and it has not been affected by HARMONY-1519.

          Show
          Elena Semukhina added a comment - This is an old issue and it has not been affected by HARMONY-1519 .
          Hide
          Alexey Varlamov added a comment -

          -1 to the suggested patch.
          1) It introduces race condition into stop(), i.e. the thread may start right after checking "alive" status and thus will not ever stop.
          2) It adds inconsistency between stop() and stop(Throwable).

          Please consider some other solution. I tried to find similar issues in Sun's bugdatabase, but in vain. I guess "alive" status should not affect this behaviour, it might be other issue.

          Show
          Alexey Varlamov added a comment - -1 to the suggested patch. 1) It introduces race condition into stop(), i.e. the thread may start right after checking "alive" status and thus will not ever stop. 2) It adds inconsistency between stop() and stop(Throwable). Please consider some other solution. I tried to find similar issues in Sun's bugdatabase, but in vain. I guess "alive" status should not affect this behaviour, it might be other issue.
          Hide
          Alexey Varlamov added a comment -

          Hmm, reading javadoc for Thread.stop(Throwable):
          "If there is a security manager installed, the checkAccess method of this thread is called, which may result in a SecurityException being raised (in the current thread).

          If this thread is different from the current thread (that is, the current thread is trying to stop a thread other than itself) or obj is not an instance of ThreadDeath, the security manager's checkPermission method (with the RuntimePermission("stopThread") argument) is called in addition. Again, this may result in throwing a SecurityException (in the current thread).

          If the argument obj is null, a NullPointerException is thrown (in the current thread). "

          Current impl of DRLVM does not follow this description.

          Note, checkPermission() for "stopThread" is called only for non-ThreadDeath throwables; and the documentation for stop() requires throwing exactly ThreadDeath instance. So there is inconsistency either in documentation or in impl of the overloaded stop() methods. I may guess stop() is equvalent of stop(new ThreadDeath()) and this is a hole in the javadoc.

          PS. I suspect order of exceptions currently is incompatible in DRLVM: NPE should be thrown last, not first.

          Show
          Alexey Varlamov added a comment - Hmm, reading javadoc for Thread.stop(Throwable): "If there is a security manager installed, the checkAccess method of this thread is called, which may result in a SecurityException being raised (in the current thread). If this thread is different from the current thread (that is, the current thread is trying to stop a thread other than itself) or obj is not an instance of ThreadDeath, the security manager's checkPermission method (with the RuntimePermission("stopThread") argument) is called in addition. Again, this may result in throwing a SecurityException (in the current thread). If the argument obj is null, a NullPointerException is thrown (in the current thread). " Current impl of DRLVM does not follow this description. Note, checkPermission() for "stopThread" is called only for non-ThreadDeath throwables; and the documentation for stop() requires throwing exactly ThreadDeath instance. So there is inconsistency either in documentation or in impl of the overloaded stop() methods. I may guess stop() is equvalent of stop(new ThreadDeath()) and this is a hole in the javadoc. PS. I suspect order of exceptions currently is incompatible in DRLVM: NPE should be thrown last, not first.
          Hide
          Alexey Varlamov added a comment -

          Please try this patch.

          Show
          Alexey Varlamov added a comment - Please try this patch.
          Hide
          Elena Semukhina added a comment -

          Alexey,

          the patch does not solve this issue - the test still fails.
          I attached a standalone test to reproduce the issue.

          Show
          Elena Semukhina added a comment - Alexey, the patch does not solve this issue - the test still fails. I attached a standalone test to reproduce the issue.
          Hide
          Elena Semukhina added a comment -

          Alexey,

          I attached the new patch which combines both suggestions. I added synchronized block to stop() as you noticed in 1) above. As for 2), the RI's behavior is asymmetric for stop() and stop(Throwable) for the threads that are not alive. The attached test ThreadStopThrowable.java demonstrates this.

          Your comments?

          Show
          Elena Semukhina added a comment - Alexey, I attached the new patch which combines both suggestions. I added synchronized block to stop() as you noticed in 1) above. As for 2), the RI's behavior is asymmetric for stop() and stop(Throwable) for the threads that are not alive. The attached test ThreadStopThrowable.java demonstrates this. Your comments?
          Hide
          Alexey Varlamov added a comment -

          Elena, thanks for the test!
          I slightly modified it and observe different behavior: indeed there is assymetry for stop() and stop(Throwable), but it does not depend on thread status. That is, there is no any check for stop() and both checkAccess(this) and checkPermission(RuntimePermission "stopThread") for stop(Throwable).
          Looking at the test_stop_subtest0, I see that exactly this behaviour is expected - I should have looked there earlier to save our time

          Show
          Alexey Varlamov added a comment - Elena, thanks for the test! I slightly modified it and observe different behavior: indeed there is assymetry for stop() and stop(Throwable), but it does not depend on thread status. That is, there is no any check for stop() and both checkAccess(this) and checkPermission(RuntimePermission "stopThread") for stop(Throwable). Looking at the test_stop_subtest0, I see that exactly this behaviour is expected - I should have looked there earlier to save our time
          Hide
          Alexey Varlamov added a comment -

          Oops, I missed some details in the test and my evaluation above is wrong. Indeed there is such queer and undocumented peculiarity around stop().

          My +1 to H-1720_combined.patch

          Show
          Alexey Varlamov added a comment - Oops, I missed some details in the test and my evaluation above is wrong. Indeed there is such queer and undocumented peculiarity around stop(). My +1 to H-1720_combined.patch
          Hide
          Geir Magnusson Jr added a comment -

          which is the right patch now?

          Show
          Geir Magnusson Jr added a comment - which is the right patch now?
          Hide
          Alexey Varlamov added a comment -

          The H-1720_combined.patch

          Show
          Alexey Varlamov added a comment - The H-1720_combined.patch
          Hide
          Geir Magnusson Jr added a comment -

          r467402.

          ubuntu 6 - smoke, c-unit, ~kernel

          Show
          Geir Magnusson Jr added a comment - r467402. ubuntu 6 - smoke, c-unit, ~kernel

            People

            • Assignee:
              Geir Magnusson Jr
              Reporter:
              Elena Semukhina
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development