Bug 872738
Summary: | Agent awaits shutdown of ProcessExecutor idle worker thread; should terminate immediately | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Other] RHQ Project | Reporter: | Elias Ross <genman> | ||||||
Component: | Agent | Assignee: | John Mazzitelli <mazz> | ||||||
Status: | CLOSED CURRENTRELEASE | QA Contact: | Mike Foley <mfoley> | ||||||
Severity: | unspecified | Docs Contact: | |||||||
Priority: | unspecified | ||||||||
Version: | unspecified | CC: | hrupp | ||||||
Target Milestone: | --- | ||||||||
Target Release: | RHQ 4.9 | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Whiteboard: | |||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||
Doc Text: | Story Points: | --- | |||||||
Clone Of: | Environment: | ||||||||
Last Closed: | 2014-03-26 08:31:20 UTC | Type: | Bug | ||||||
Regression: | --- | Mount Type: | --- | ||||||
Documentation: | --- | CRM: | |||||||
Verified Versions: | Category: | --- | |||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||
Embargoed: | |||||||||
Attachments: |
|
Description
Elias Ross
2012-11-02 20:48:53 UTC
Or try setting the thread to a daemon one. Although you may not really want that thread terminated if the agent is running some sort of shell command. You can also set the timeout for pooled threads to be a lot smaller than 60 seconds. Or remove the 'static' declaration. Created attachment 639678 [details]
incomplete patch, but addresses some of the design
The easiest fix is probably okay, but references to ProcessExecutor should probably be retained otherwise a thread pool is created on every exec. I changed the classes to hold a reference to a ProcessExecutor.
But still the issue is the Agent container should really be responsible and shut down the exec thread pool.
How can this hook be created? Could ResourceContainer's thread pool be used? Etc.
Another issue if the stream I/O threads are really daemon or not.
Created attachment 763100 [details]
This is the complete suggested fix
The only complaint I have is, is that SystemInfoFactory.shutdown() should really be a final shutdown.
In org.rhq.core.util.exec.StreamRedirector.StreamRedirector(String, InputStream, OutputStream), I see this has now been removed (because it is no longer extending Thread, but now only implementing Runnable): setDaemon(true); However, I don't see the threads created by the thread factory to be daemon threads (the default, I believe, is to be non-daemon). In order to get back to the original behavior, I think we need to add: t.setDaemon(true); to: org.rhq.core.system.SystemInfoFactory.threadFactory.new ThreadFactory() {...}.newThread(Runnable) after line 72: public Thread newThread(Runnable r) { Thread t = new Thread(r); t.setName("systeminfo-" + threadNumber.getAndIncrement()); t.setDaemon(true); return t; } However, I am not going to do this now because I think the StreamRedirector threads are getting created by the same executor/thread pool that is running everything else (which I don't think we want to be daemon). The question becomes - what happens if these stream redirect threads hang? It used to be it wouldn't matter - the agent VM will still go down because these were daemon threads. But now they are not. Is is possible this new shutdown code could leave some non-daemon threads hanging around? (In reply to Elias Ross from comment #3) > Created attachment 763100 [details] > This is the complete suggested fix > > The only complaint I have is, is that SystemInfoFactory.shutdown() should > really be a final shutdown. I pushed this to master branch - git commit : b361a1b07c7d14834fcf4986e09f7bf683be4bf8 the patch has been pushed to master and thus will be in RHQ 4.9. Moving this to ON_QA Bulk closing now that 4.10 is out. If you think an issue is not resolved, please open a new BZ and link to the existing one. |