Qpid
  1. Qpid
  2. QPID-3325

make the ApplicationRegistry unregister its shutdown hook when closed by means other than the shutdown hook

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.15
    • Component/s: Java Broker
    • Labels:
      None
    1. QPID-3325.patch
      1 kB
      Danushka Menikkumbura

      Activity

      Hide
      Keith Wall added a comment -

      Changes look fine to me.

      Show
      Keith Wall added a comment - Changes look fine to me.
      Hide
      Robbie Gemmell added a comment -

      I had thought about putting it in Broker, so I guess you doing so too means that I should have. I didnt name the thread as I dont really think it really makes any real difference in this case, but that works both ways so I'm happy to do so. Changes made.

      Show
      Robbie Gemmell added a comment - I had thought about putting it in Broker, so I guess you doing so too means that I should have. I didnt name the thread as I dont really think it really makes any real difference in this case, but that works both ways so I'm happy to do so. Changes made.
      Hide
      Keith Wall added a comment -

      Hi Robbie,

      Two minor comments:

      1) I think we should set the ShutdownHook's thread's name. We seen the occasional deadlocks during shutdown in the past, and having the thread name in the thread dump might help diagnose if we ever have a re-occurence.

      2) I'm wondering if the responsibility for creating/registering/deregistering the shutdown hook would better sit with Broker rather than ApplicationRegistry. (Maybe this would go beyond the scope of this Jira. What do you think)?

      cheers Keith

      Show
      Keith Wall added a comment - Hi Robbie, Two minor comments: 1) I think we should set the ShutdownHook's thread's name. We seen the occasional deadlocks during shutdown in the past, and having the thread name in the thread dump might help diagnose if we ever have a re-occurence. 2) I'm wondering if the responsibility for creating/registering/deregistering the shutdown hook would better sit with Broker rather than ApplicationRegistry. (Maybe this would go beyond the scope of this Jira. What do you think)? cheers Keith
      Hide
      Robbie Gemmell added a comment -

      Keith, could you review this change please?

      Thanks,
      Robbie

      Show
      Robbie Gemmell added a comment - Keith, could you review this change please? Thanks, Robbie
      Hide
      Robbie Gemmell added a comment -

      Given the lack of reply to the above questions I think its past time we just make the change we thought was most suitable and have the ApplicationRegistry remove its shutdown hook in the event it is closed by something other than the hook. This would allow an embeeded broker to be shut down without it leaving behind a shutdown hook that would later run when the JVM was shut down, which I believe is the only reasonable explanation behind the request to directly access the thread.

      Changing title from 'Make it possible to access broker shutdown hook from outside' to 'make the ApplicationRegistry unregister its shutdown hook when closed by means other than the shutdown hook' to reflect the actual change being made.

      Show
      Robbie Gemmell added a comment - Given the lack of reply to the above questions I think its past time we just make the change we thought was most suitable and have the ApplicationRegistry remove its shutdown hook in the event it is closed by something other than the hook. This would allow an embeeded broker to be shut down without it leaving behind a shutdown hook that would later run when the JVM was shut down, which I believe is the only reasonable explanation behind the request to directly access the thread. Changing title from 'Make it possible to access broker shutdown hook from outside' to 'make the ApplicationRegistry unregister its shutdown hook when closed by means other than the shutdown hook' to reflect the actual change being made.
      Hide
      Robbie Gemmell added a comment -

      Hi Danushka,

      The work Keith referenced (QPID-3026) has now been completed, in concert with other related work on broker startup (QPID-2815). Keith's suggestion from this this JIRA was not included, but I think its a good idea and should be. My main reservation with the attached patch would be that since none of our code uses the getter and its not obvious why it should be there then it would likely just get 'cleaned up' by someone at some point.

      Could you let us know if the reasoning behind exposing the shutdown hook thread was as queried, and whether the suggested approach would do what you are looking for? (updated patch welcome too)

      Thanks,
      Robbie

      Show
      Robbie Gemmell added a comment - Hi Danushka, The work Keith referenced ( QPID-3026 ) has now been completed, in concert with other related work on broker startup ( QPID-2815 ). Keith's suggestion from this this JIRA was not included, but I think its a good idea and should be. My main reservation with the attached patch would be that since none of our code uses the getter and its not obvious why it should be there then it would likely just get 'cleaned up' by someone at some point. Could you let us know if the reasoning behind exposing the shutdown hook thread was as queried, and whether the suggested approach would do what you are looking for? (updated patch welcome too) Thanks, Robbie
      Hide
      Keith Wall added a comment -

      Hi Danushka

      I've reviewed your patch.

      First, I have a question. For what purpose does calling code require access to the ShutdownServiceThread? If it is so the calling code can can unregister the shutdown hook, then I think it would be better if ApplicationRegistry itself took on this responsibility. I think ApplicationRegistry should register the shutdown hook on initialisation (initialise()), and remove the shutdown hook when the remove is called (currently remove/removeAll()).

      I am currently working in ApplicationRegistry (QPID-3026) and could incorporate this improvement too. Please let me know your thoughts.

      Kind regards Keith Wall.

      Show
      Keith Wall added a comment - Hi Danushka I've reviewed your patch. First, I have a question. For what purpose does calling code require access to the ShutdownServiceThread? If it is so the calling code can can unregister the shutdown hook, then I think it would be better if ApplicationRegistry itself took on this responsibility. I think ApplicationRegistry should register the shutdown hook on initialisation (initialise()), and remove the shutdown hook when the remove is called (currently remove/removeAll()). I am currently working in ApplicationRegistry ( QPID-3026 ) and could incorporate this improvement too. Please let me know your thoughts. Kind regards Keith Wall.
      Hide
      Danushka Menikkumbura added a comment -

      Ability to access broker shutdown hook from outside the ApplicationRegistry is quite useful when broker runs in embedded mode.

      Show
      Danushka Menikkumbura added a comment - Ability to access broker shutdown hook from outside the ApplicationRegistry is quite useful when broker runs in embedded mode.

        People

        • Assignee:
          Keith Wall
          Reporter:
          Danushka Menikkumbura
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development