Tapestry 5
  1. Tapestry 5
  2. TAP5-1287

Some services require a notification that they have been reloaded, so they can clean up external dependencies

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.2.2
    • Fix Version/s: 5.2.2
    • Component/s: tapestry-ioc
    • Labels:
      None

      Description

      Been working on an application that uses a lot of JMS. I noticed that reloads did not appear to happen as expected.

      DEBUG MessageSinkSource - Invoking constructor public com.fivoosh.services.activemq.MessageSinkSourceImpl(org.slf4j.Logger,com.fivoosh.services.activemq.ActiveMQConnectionSource,org.apache.tapestry5.ioc.services.PerthreadManager,com.fivoosh.services.TimeService,java.util.Map).
      DEBUG MessageSinkSource - BEGIN Analyzing com.fivoosh.services.activemq.MessageSinkSourceImpl$1
      DEBUG MessageSinkSource - END Analyzing com.fivoosh.services.activemq.MessageSinkSourceImpl$1
      DEBUG MessageSinkSource - BEGIN Analyzing com.fivoosh.services.activemq.MessageSinkSourceImpl$TextQueueSender
      DEBUG MessageSinkSource - END Analyzing com.fivoosh.services.activemq.MessageSinkSourceImpl$TextQueueSender
      DEBUG MasterMessageHandler - Processing message 'ID:Howards-Mighty-Tool.local-56885-1285896586008-3:0:9:1:1' from queue 'echo'
      Message received on thread ActiveMQ Session Task:

      { "foo" : "bar" }

      I then changed the code (it affects the logging message that starts "Processing message ....":

      DEBUG MasterMessageHandler - Implementation class com.fivoosh.services.activemq.MasterMessageHandlerImpl has changed and will be reloaded on next use.
      DEBUG MasterMessageHandler - Processing message 'ID:Howards-Mighty-Tool.local-56885-1285896586008-3:0:10:1:1' from queue 'echo'
      Message received on thread ActiveMQ Session Task:

      { "foo" : "bar" }

      Note that the message indicates the implementation class changed, but the old behavior is stuck.

      Later changes to the code and new messages sent:

      DEBUG MasterMessageHandler - Processing message 'ID:Howards-Mighty-Tool.local-56885-1285896586008-3:0:12:1:1' from queue 'echo'
      Message received on thread ActiveMQ Session Task:

      { "foo" : "baz" }

      ... do not even register that a change occured.

        Activity

        Hide
        Hudson added a comment -

        Integrated in tapestry-5.2-freestyle #201 (See https://hudson.apache.org/hudson/job/tapestry-5.2-freestyle/201/)
        TAP5-1287: Some services require a notification that they have been reloaded, so they can clean up external dependencies
        TAP5-1287: Services do not reload even though they should

        Show
        Hudson added a comment - Integrated in tapestry-5.2-freestyle #201 (See https://hudson.apache.org/hudson/job/tapestry-5.2-freestyle/201/ ) TAP5-1287 : Some services require a notification that they have been reloaded, so they can clean up external dependencies TAP5-1287 : Services do not reload even though they should
        Hide
        Howard M. Lewis Ship added a comment -

        Ah, the JMS is the critical issue.

        What's happened is that the code change is observed by Tapestry and it discards the current service instance.

        However, because of what this service does (it obtains a JMS QueueSession, then a Queue, then a MessageConsumer and then sets the consumer's message listener to an inner class instance.

        That's the trick: the service has exposed its implementation to the outside world (the JMS code). The JMS code directly invokes methods on the service implementation (or inner classes provided by the service implementation). Since nothing is going through the proxy, the new version of the code is not reloaded.

        Tapestry is missing a callback that would allow the existing service implementation to cleanly shut down before being replaced.

        I see this as an optional interface that could be implemented by a service implementation (or, for that matter, an non-service proxy object):

        public interface ReloadAware

        { void shutdownInstanceBeforeReload(); }

        It might make sense to have a second method to alert the new instance that it as been reloaded, but I'm not sure when that would be used, so I'll leave it out for now. Further, I've given
        some thought to services that have some internal state (such as a list of event listener objects) that needs to persist between the old and new instances (i picture a Map being passed to the old instance and then the new instance); again, I'd rather play conservative and introduce a more complicated interface that supports those features only as necessary.

        Show
        Howard M. Lewis Ship added a comment - Ah, the JMS is the critical issue. What's happened is that the code change is observed by Tapestry and it discards the current service instance. However, because of what this service does (it obtains a JMS QueueSession, then a Queue, then a MessageConsumer and then sets the consumer's message listener to an inner class instance. That's the trick: the service has exposed its implementation to the outside world (the JMS code). The JMS code directly invokes methods on the service implementation (or inner classes provided by the service implementation). Since nothing is going through the proxy, the new version of the code is not reloaded. Tapestry is missing a callback that would allow the existing service implementation to cleanly shut down before being replaced. I see this as an optional interface that could be implemented by a service implementation (or, for that matter, an non-service proxy object): public interface ReloadAware { void shutdownInstanceBeforeReload(); } It might make sense to have a second method to alert the new instance that it as been reloaded, but I'm not sure when that would be used, so I'll leave it out for now. Further, I've given some thought to services that have some internal state (such as a list of event listener objects) that needs to persist between the old and new instances (i picture a Map being passed to the old instance and then the new instance); again, I'd rather play conservative and introduce a more complicated interface that supports those features only as necessary.
        Hide
        Howard M. Lewis Ship added a comment -

        Actually, reviewing where I was making code changes, it was a service (and not a proxy) that was not being reloaded. I think that is worse.

        Show
        Howard M. Lewis Ship added a comment - Actually, reviewing where I was making code changes, it was a service (and not a proxy) that was not being reloaded. I think that is worse.
        Hide
        Howard M. Lewis Ship added a comment -

        Hm. We do have an existing test for this:

        @Test
        public void reload_a_proxy_object() throws Exception

        { createImplementationClass("initial proxy"); Registry registry = createRegistry(); Class<ReloadableService> clazz = (Class<ReloadableService>) classLoader.loadClass(CLASS); ReloadableService reloadable = registry.proxy(ReloadableService.class, clazz); assertEquals(reloadable.getStatus(), "initial proxy"); touch(classFile); createImplementationClass("updated proxy"); fireUpdateCheck(registry); assertEquals(reloadable.getStatus(), "updated proxy"); touch(classFile); createImplementationClass("re-updated proxy"); fireUpdateCheck(registry); assertEquals(reloadable.getStatus(), "re-updated proxy"); registry.shutdown(); }
        Show
        Howard M. Lewis Ship added a comment - Hm. We do have an existing test for this: @Test public void reload_a_proxy_object() throws Exception { createImplementationClass("initial proxy"); Registry registry = createRegistry(); Class<ReloadableService> clazz = (Class<ReloadableService>) classLoader.loadClass(CLASS); ReloadableService reloadable = registry.proxy(ReloadableService.class, clazz); assertEquals(reloadable.getStatus(), "initial proxy"); touch(classFile); createImplementationClass("updated proxy"); fireUpdateCheck(registry); assertEquals(reloadable.getStatus(), "updated proxy"); touch(classFile); createImplementationClass("re-updated proxy"); fireUpdateCheck(registry); assertEquals(reloadable.getStatus(), "re-updated proxy"); registry.shutdown(); }
        Hide
        Howard M. Lewis Ship added a comment -

        This could represent the difference between service reloading and proxy reloading; the objects in question were created via ObjectLocator.proxy().

        Show
        Howard M. Lewis Ship added a comment - This could represent the difference between service reloading and proxy reloading; the objects in question were created via ObjectLocator.proxy().

          People

          • Assignee:
            Howard M. Lewis Ship
            Reporter:
            Howard M. Lewis Ship
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development