Wicket
  1. Wicket
  2. WICKET-4285

PageSavingThread.stop() blocks forever

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.19
    • Fix Version/s: 1.4.20, 1.5.4, 6.0.0-beta1
    • Component/s: wicket
    • Labels:

      Description

      The PageSavingThread may exit without nulling the stop flag. For example when an OutOfMemoryError or any other runtime exception is thrown in the run method.
      Properly set the flag in a finally clause.

      Is this class still present in 1.5?

      1. patch-1.5.diff
        3 kB
        Ortwin Glück
      2. patch.diff
        3 kB
        Ortwin Glück

        Activity

        Hide
        Ortwin Glück added a comment -

        put it in a finally clause

        Show
        Ortwin Glück added a comment - put it in a finally clause
        Hide
        Martin Grigorov added a comment -

        In 1.5 see AsynchronousDataStore

        Show
        Martin Grigorov added a comment - In 1.5 see AsynchronousDataStore
        Hide
        Ortwin Glück added a comment -

        In 1.5 it's even worse. AsynchronousDataStore.destroy() does an unconditional wait() on the destroy object. The thread may have already exited, and there is no way to detect that.
        (Effective Java item 50: never invoke wait() outside a loop, was written exactly for this reason).

        Why this trickery with wait/notify if Threads already have isAlive() and join() methods? I suggest to simply keep a reference to the thread. Patch imminent.

        Show
        Ortwin Glück added a comment - In 1.5 it's even worse. AsynchronousDataStore.destroy() does an unconditional wait() on the destroy object. The thread may have already exited, and there is no way to detect that. (Effective Java item 50: never invoke wait() outside a loop, was written exactly for this reason). Why this trickery with wait/notify if Threads already have isAlive() and join() methods? I suggest to simply keep a reference to the thread. Patch imminent.
        Hide
        Ortwin Glück added a comment -

        patch for 1.5.x

        Show
        Ortwin Glück added a comment - patch for 1.5.x
        Hide
        Ortwin Glück added a comment -

        By the way, this is not a theoretical problem. I encountered this in practice: A ClassNotFoundException (don't worry about the reasons) caused the thread to die. I had then to kill the application server because it refused to shut down properly. Wicket should not prevent my app server from shutting down.

        Show
        Ortwin Glück added a comment - By the way, this is not a theoretical problem. I encountered this in practice: A ClassNotFoundException (don't worry about the reasons) caused the thread to die. I had then to kill the application server because it refused to shut down properly. Wicket should not prevent my app server from shutting down.
        Hide
        Martin Grigorov added a comment -

        Your fix for 1.5 breaks the unit tests. They hang now. See org.apache.wicket.page.persistent.disk.DiskDataStoreTest#store
        The patch for 1.4 is not very clear. Did you just wrap it with try/finally

        {stop = null; }

        ?

        Show
        Martin Grigorov added a comment - Your fix for 1.5 breaks the unit tests. They hang now. See org.apache.wicket.page.persistent.disk.DiskDataStoreTest#store The patch for 1.4 is not very clear. Did you just wrap it with try/finally {stop = null; } ?
        Hide
        Ortwin Glück added a comment -

        patch for 1.5 that passes the test case

        Show
        Ortwin Glück added a comment - patch for 1.5 that passes the test case
        Hide
        Ortwin Glück added a comment -

        Martin, yes the 1.4 patch is as trivial as you think it is

        Show
        Ortwin Glück added a comment - Martin, yes the 1.4 patch is as trivial as you think it is

          People

          • Assignee:
            Martin Grigorov
            Reporter:
            Ortwin Glück
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development