BZ966777 and interruptible component invocation

Lukas Krejci lkrejci at redhat.com
Mon Jul 1 12:54:07 UTC 2013


On Monday, July 01, 2013 14:32:04 Thomas Segismont wrote:
> Lukas,
> 
> Please find my answers below
> 
> Le 01/07/2013 13:08, Lukas Krejci a écrit :
> > I have one question and one comment :)
> > 
> > 1) What is the advantage of ComponentInvocationContext over
> > Thread.isInterrupted() and Thread.interrupt()?
> 
> ComponentInvocationContext is an RHQ concept and has very few chances to
> get messed up.
> 
> This is not the case with Thread interruption. Many plugin component
> implementations use libraries and these libraries could clear the
> interrupted status before the component gets a chance to check it.
> 

I agree with what you say, but am not sure if having "one more thing to 
remember" to do things is the right thing to do ;) I mean this 
ComponentInvocationContext is virtually undiscoverable unless one reads 
documentation.

That said I understand and agree with your reasoning, I'm just not sure if 
"thread interruption gets messed up" is strong enough an argument for us to 
introduce a whole new concept - maybe supporting both standard thread 
interruption and this invocation context would be the best thing (or do we do 
that already - haven't checked the code in that much detail)?

> > 2) I'd personally split the ComponentInvocationContext into 2 classes - 1
> > visible in the plugin API and the other only in the plugin container.
> > I think ComponentInvocationContext.remove() and setCurrent() methods
> > should
> > not be used by the plugins or have I misunderstood their purpose?
> 
> Right it's not intended that plugins use
> ComponentInvocationContext.remove() and setCurrent().
> 
> If we want to do this we could have a new
> getComponentInvocationContext() in ResourceContext class.
> 

I think that would be the best place for it - a standard location people are 
used to get the "contextual stuff" from.

> Any other idea?
> 
> > On Monday, July 01, 2013 10:10:30 Thomas Segismont wrote:
> >> Hi,
> >> 
> >> I've pushed my changes to a bug branch. Can someone please have a look
> >> at it before it gets merged?
> >> 
> >> https://git.fedorahosted.org/cgit/rhq/rhq.git/commit/?h=bug/966777&id=7b7
> >> f0e ff22213847156e9262090f65f845842185
> >> 
> >> This first commit adds a new transferInterrupt parameter to
> >> ResourceContainer#createResourceComponentProxy
> >> 
> >> Here is the Javadoc:
> >> 
> >> +     * @param transferInterrupt whether or not interruption of the
> >> calling thread should be transferred to the executor
> >> +     *                          thread
> >> 
> >> https://git.fedorahosted.org/cgit/rhq/rhq.git/commit/?h=bug/966777&id=a6c
> >> 62d 9bd0c8d848c2770892872f4ee200e059f0
> >> 
> >> The second commit introduces the ComponentInvocationContext class. An
> >> instance of this class is created by the plugin container and bound to
> >> facet-locked component invocation thread.
> >> 
> >> This commit also fixes the actual problem of BZ966777 (EAP 6 plug-in is
> >> using a hard-coded operation timeout for start and stop).
> >> 
> >> Thanks and regards,
> >> Thomas
> >> 
> >> Le 17/06/2013 17:25, Thomas Segismont a écrit :
> >>> Hi everyone,
> >>> 
> >>> I'm working on BZ966777 [1]: "EAP 6 plug-in is using a hard-coded
> >>> operation timeout for start and stop instead of using the operation
> >>> timeout or agent's default operation timeout of 10 minutes"
> >>> 
> >>> The reason why the start operation of this plugin is failing after 15
> >>> seconds is that the implementation of BaseServerComponent#startServer
> >>> creates a ProcessExecution and then calls
> >>> "processExecution.setWaitForCompletion(15000L);"
> >>> 
> >>> Removing this call is necessary as part of the fix but it won't be
> >>> enough. After the SystemInfo instance has been asked to execute the
> >>> process, it is checked if the AS7 server has started in
> >>> BaseServerComponent#waitForServerToStart. This method tries up to 20
> >>> times to read an attribute of the root node of the server management
> >>> tree. The invocation thread sleeps one second between tries.
> >>> Consequently, with slow starting servers the operation would still be
> >>> marked as failed after just over 20 seconds.
> >>> 
> >>> So I read the OperationFacet contract. Here's an extract of the
> >>> OperationFacet#invokeOperation Javadoc:
> >>> 
> >>> "Plugin developers must write implementations of this method with the
> >>> awareness that operations are cancelable and should prepare for such a
> >>> possibility in the code. For example, a user may request that an
> >>> operation be canceled, or an operation may be taking too long and it
> >>> should be canceled due to a timeout expiration. When an operation
> >>> invocation is canceled, the thread that is invoking this method will be
> >>> interrupted. [...] When an operation invocation is canceled, the thread
> >>> that is invoking this method will be interrupted. [...] Plugin writers
> >>> are encouraged to check the current thread's Thread.isInterrupted()
> >>> method periodically to catch cancellation requests, as well as catching
> >>> and processing InterruptedException appropriately."
> >>> 
> >>> My understanding is that canceled and timed out operations should result
> >>> in the invocation thread being interrupted. Neither is happening.
> >>> 
> >>> OperationInvocation#markAsCanceled actually interrupts a thread from the
> >>> "OperationManager.invoker" pool. But it's not this one which executes
> >>> the operation, it's a thread from the
> >>> "ResourceContainer.invoker.nonDaemon" pool, because the resource
> >>> component is invoked through a proxy. And in
> >>> ResourceComponentInvocationHandler#invokeInNewThreadWithLock, only
> >>> daemon threads are interrupted when the caller thread is interrupted.
> >>> 
> >>> So far, there's a couple of questions I wasn't able to answer:
> >>> 
> >>> * why do we have two different pools in
> >>> ResourceComponentInvocationHandler?
> >>> 
> >>> These pools appeared in commit fb3a048 [2] and before that, every
> >>> ComponentInvocationThread was a daemon thread.
> >>> 
> >>> * why do we transfer the "interrupt" to daemon threads only?
> >>> 
> >>> Any ideas?
> >>> 
> >>> In addition, I think that calling interrupt to tell a component that an
> >>> operation was canceled is not a strong enough design. Plugins developers
> >>> use libraries to implement operations (so do we). The interrupted status
> >>> of the thread could be cleared by a library. I would like to introduce a
> >>> new method to the ResourceContext class:
> >>> 
> >>> ComponentInvocationContext getComponentInvocationContext();
> >>> 
> >>> And the ComponentInvocationContext interface:
> >>> 
> >>> public interface ComponentInvocationContext {
> >>> 
> >>>     boolean isCanceled();
> >>> 
> >>> }
> >>> 
> >>> Then plugin developers would only have to check.
> >>> 
> >>> What's you opinion?
> >>> 
> >>> Thanks and regards,
> >>> Thomas
> >>> 
> >>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=966777
> >>> [2]
> >>> https://git.fedorahosted.org/cgit/rhq/rhq.git/commit/?id=fb3a0489469062b
> >>> ac
> >>> 9db053389481f7d80babd8b
> >>> 
> >>> _______________________________________________
> >>> rhq-devel mailing list
> >>> rhq-devel at lists.fedorahosted.org
> >>> https://lists.fedorahosted.org/mailman/listinfo/rhq-devel
> >> 
> >> _______________________________________________
> >> rhq-devel mailing list
> >> rhq-devel at lists.fedorahosted.org
> >> https://lists.fedorahosted.org/mailman/listinfo/rhq-devel
> > 
> > _______________________________________________
> > rhq-devel mailing list
> > rhq-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/rhq-devel
> 
> _______________________________________________
> rhq-devel mailing list
> rhq-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/rhq-devel


More information about the rhq-devel mailing list