Details

    • Improvement
    • Status: Resolved
    • Low
    • Resolution: Not A Problem
    • 2.0.0
    • None
    • None

    Description

      There are a lot of catch-wrap-throw pattern occurances in code with InterruptedException. I cleaned up some of them but not all - in some places I don't know enough about code to decide if it's correct thing to do.

      Important: I also fixed possibility of wrong behaviour in case of spurious wakeup in AsyncOneResponse.

      Attachments

        1. trunk-5623.txt
          41 kB
          Mikhail Mazursky

        Issue Links

          Activity

            dbrosius David Brosius added a comment -

            This changes the semantics of what was going on. I can see using Futures.getUnchecked etc, to clean the code up some. I'm not sure I understand the intention of this patch, as it makes this non interruptable, which is completely different, and to me seems wrong. But perhaps it went over my head.

            dbrosius David Brosius added a comment - This changes the semantics of what was going on. I can see using Futures.getUnchecked etc, to clean the code up some. I'm not sure I understand the intention of this patch, as it makes this non interruptable, which is completely different, and to me seems wrong. But perhaps it went over my head.
            ash2k Mikhail Mazursky added a comment - - edited

            AFAIK there are two basic ways of dealing with interruption - ignore it (remember that you were interrupted, continue with the task and restore the interruption state after you're done) or propagate it as InterruptedException up the call chain (rarely it's wrapped in another exception when there is no other way e.g. InterruptedIOException). Both ways allow calling code to learn that the Thread was interrupted.

            Wrapping IE in AssertionError/RuntimeException is clearly wrong - interruption is not an emergency way to stop thread, it's a polite way to ask to consider stopping current activity, which can be ignored if you can't or don't want to handle it.

            But, yes, this patch changes semantics/behaviour. Probably in some places code can/should be refactored to propagate that exception instead of ignoring it.

            The intention of this patch is to make code more correct, improve it's quality. And also I'm learning the codebase that way.

            ash2k Mikhail Mazursky added a comment - - edited AFAIK there are two basic ways of dealing with interruption - ignore it (remember that you were interrupted, continue with the task and restore the interruption state after you're done) or propagate it as InterruptedException up the call chain (rarely it's wrapped in another exception when there is no other way e.g. InterruptedIOException). Both ways allow calling code to learn that the Thread was interrupted. Wrapping IE in AssertionError/RuntimeException is clearly wrong - interruption is not an emergency way to stop thread, it's a polite way to ask to consider stopping current activity, which can be ignored if you can't or don't want to handle it. But, yes, this patch changes semantics/behaviour. Probably in some places code can/should be refactored to propagate that exception instead of ignoring it. The intention of this patch is to make code more correct, improve it's quality. And also I'm learning the codebase that way.
            dbrosius David Brosius added a comment -

            thanks for the clarification.

            my concern is that how do i, in a code review, determine if this doesn't introduce infinite loops, that were once broken out of by the IE. Also, fail fast tends to have safer/easier to recover from behavior.

            This patch might be fine, I'm just not sure how to prove it to myself.

            dbrosius David Brosius added a comment - thanks for the clarification. my concern is that how do i, in a code review, determine if this doesn't introduce infinite loops, that were once broken out of by the IE. Also, fail fast tends to have safer/easier to recover from behavior. This patch might be fine, I'm just not sure how to prove it to myself.

            I opened another more specific issue CASSANDRA-5690 to patch problems in AsyncOneResponse. Probably this issue can be closed.

            ash2k Mikhail Mazursky added a comment - I opened another more specific issue CASSANDRA-5690 to patch problems in AsyncOneResponse. Probably this issue can be closed.

            People

              ash2k Mikhail Mazursky
              ash2k Mikhail Mazursky
              Mikhail Mazursky
              David Brosius
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: