Bugzilla – Full Text Bug Listing
| Summary: | Avoid usage of HierarchyResourceSetImpl.getInstance() due to memory leak issues. | ||
|---|---|---|---|
| Product: | z_Archived | Reporter: | Rohit Shetty <rohit.shetty> |
| Component: | TPTP | Assignee: | Sheldon Lee-Loy <sleeloy> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | critical | ||
| Priority: | P1 | CC: | apnan, ewchan, igururao, labadie, prafulr, sleeloy, smith, umarkova, zung |
| Version: | unspecified | Keywords: | plan |
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | closed460 | ||
|
Description
Rohit Shetty
HierarchyResourceSetImpl.getInstance() is being used in code from the packages mentioned earlier. And also in support code from the models package like org.eclipse.hyades.loaders.util.LoaderUtils etc. We need to ensure that this is not used anywhere to avoid memory leaks. Another way to avoid this problem is to change the HierarchyResourceSetImpl.getInstance() to use HierarchyResourceSetImpl.getInstance(resourceID) and this resourceID would be used to fetch the right resourceSet for a user/namespace. I would suggest using the above impl since it would allow a standard way to create resourceSets just like HierarchyResourceSetImpl.getInstance() allows today. If this is adopted then there would have to be a way to pass this resourceID to the code that uses this. The fix is required in 4.3 stream. (In reply to comment #2) Are you proposing for the HierarchyResourceSetImpl class to hold to a map of resource sets keyed on resourceID? Wouldn't there be a memory leak in this cases since this map would increase over time. Most of the common services uses a factory method to constuct the facade interface. Maybe we can pass in the resourceID or resourceSet to the factory method. The factory method would replace the HierarchyResourceSetImpl.getInstance() with the supplied resourceSet. Sheldon, would you please follow up on this? Rohit, I propose fixing this bug in TPTP 4.3 i3 since more thought is needed on this issue. This will also effect quite a bit of code. I am more confortable to have this done in the next iteration. Do you agree? According to the TPTP schedule (http://web.cs.opensource.ibm.com/www/ac-tech/projectManagement/ACLT4.2/schedule/4.3/AC4.3-Milestones-Status-9-26-06.html), TPTP v.4.3i3 defect fix phase ends this Friday. Next week is for stop-ship only. Does your proposal to fix the defect in TPTP v.4.3i3 mean that it will be fixed by this Friday? (In reply to comment #7) Here is the correct schedule link: http://www.eclipse.org/tptp/home/project_info/releaseinfo/4.3/schedule.html We are currently ending TPTP 4.3i2. According to the schedule we have 2 weeks in 4.3i3 to fix sev1 and sev2 bugs. Note that the dates aren't correct since we are delayed in the i2 release. (In reply to Comment #4 ) Yes that is one of the solutions. If that is done then it would be the responsibility of the code managing the resourceSet to ensure that the resourceSet used by it is unloaded and cleaned up at the right time. Thats how it works in the PLA, We have one resourceSet per user and we load/unload the users resourceSet based on the operations performed by the user (login/logout). The code that uses the HierarchyResourceSetImpl singleton creates a problem for us since we will not be able to handle the resources for each user and we are stuck with these resources until the application is killed or all users logoff. Also keep in mind that the code should be able to handle multiple users and locales at a time. What i mean by this is that any code that is static has a very high possibility of creating problems in a multiuser environment. The locale is also important, since we handle situations where each user logging into our application might have a different locale configuration and all NLS content will need to be displayed in that locale for each user. The OSGi resource bundle framework has a problem with this (maintains static instance, which means that there will be only one language) and SHOULD NOT be used. In our application we use a class UserInfo that tracks this info for each user. Maybe even you could use something like this. Everyone callingh your code would pass this object to you and this would have information on the ResourceSet and Locale etc .... For eclipse LTA you could create a base implementation that would use the HierarchyResourceSetImpl singleton for the resourceSet and Locale.getDefault() for the locale. What do you think? Are these the only issues that are preventing the PLA from consuming the common service APIs? I am thinking of creating a context object to pass the resource set and locale to the common service api. Therefore one would pass the resourceset and locale as follows: IOperationContext context = new OperationContextImpl(); context.setProperty(CONTEXT_LOCAL_PROPERTY, Locale.getDefault()); context.setProperty(CONTEXT_RESOURCE_SET, myResourceSet); IImportHandler handler = CBEServiceFactoryWrapper.instance().createImportHandler(element, context); If the CONTEXT_LOCAL_PROPERTY or CONTEXT_RESOURCE_SET are not set the Locale.getDefault() and HierarchyResourceSetImpl.getInstance() are used. The above example shows the log import scenario. The other scenarios such as log export, sdb import/export, correlation and analysis will follow a similar workflow. Could you email me directly whether you are working with the common service apis right now. I would like to get information on your progress so that I might help you integrate with these apis. Could you also tell me the specific issues with using the OSGI resource bundle framework?
Some of the code also uses java.util.ResourceBundle. The following is a sample class.
public class Messages {
private static final String BUNDLE_NAME = "org.eclipse.tptp.monitoring.log.internal.export.messages"; //$NON-NLS-1$
private static final ResourceBundle RESOURCE_BUNDLE = ResourceBundle
.getBundle(BUNDLE_NAME);
private Messages() {
}
public static String getString(String key) {
try {
return RESOURCE_BUNDLE.getString(key);
} catch (MissingResourceException e) {
return '!' + key + '!';
}
}
}
So the issue is that the ResourceBundle RESOURCE_BUNDLE is static and the fact that the Locale.getDefault is used?
I am wondering how the PLA currenlty gets around this problem if the PLA is reusing the TPTP runtime since this method of externalizing strings is used all through the TPTP code.
Sheldon, Yes that is exactly the problem. static instance would create problems. With the OSGi resource bundle that problem is that the initialization happens only once when the class is loaded for the first time and all the variables are static. So its always initialized with one language. org.eclipse.tptp.monitoring.logui\src\org\eclipse\tptp\monitoring\logui\internal\LogUIMessages.java is an example. As of today we do not expose a lot of messages that originate in the tptp code. But what ever is originated from there and exposed to the user is shown in the language the container is deployed in (currently a limitation). In reply to Comment #6 After looking at the comments in Comment #8 getting the fix in TPTP 4.3 i3 would be fine by us. Changing priority to reflect seriousness. I have a fix in hand that was also reviewed and accepted by Rohit. The fix invovles changes to 8 classes and 3 new classes. A new mechanism is provided to allow consuming products to provide the resource set. Specifically, the resource set is passed to the common service apis via the context object. Two new context property is provided. That is the user will specify a session id and a session manager that will provide the resource set. I changed the context prooperties as follows IOperationContext context = new OperationContextImpl(); context.setProperty(CONTEXT_SESSION_MANAGER, mySessionManager); context.setProperty(CONTEXT_SESSIONID, "mysessionID"); mySessionManager will implement an interface to get the resource set based on sessionid. If no resource set is provided the HierarchyResourceSetImpl is used (This is how it worked before). This fix will get rid of the memory leak found in the way consuming products are using the common service apis. Added Dave Smith to the bug to also review this bug. For the locale problem I suggest this get fixed in 4.4 since it is a less serious problem and in 4.3 i3 we want to reduce the amount of changes we get into this release. Rohit open a new bug regarding the locale problem if you agree with this assessment. As discussed moving target to 4.4 Added me in cc list Design Note: In regards to the ResourceBundle problem I will not change the apis to get the messages for any of the error strings. This change is too huge and error prone. I am only going to change the resource bundle apis that will effect what the end user will see. For example the export apis for the logs (i.e. the csv, xml and html reports). Creation of EMF Resources ============================ In a multiple session environment the resources used by the Common Services should not be shared across sessions. For this reason the org.eclipse.hyades.models.hierarchy.util.ISessionManager interface is provided that manages the resource set creation based on a session id. This interface provides the following API: /** * Creates a resource based on a unique identifier. * @param sessionID a string that identifies a session * @return a resource set based on a unique identifier */ public ResourceSet getResourceSet(String sessionID); The getResourceSet API should be implemented and return a resource set based on a specific session id. Note the resource set that is returned should have the ability to create the various TPTP EMF resources such as TRCAgents, TRCNodes, TRCMonitors, TRCProcesses, Symptom Databases, and Correlations. An instance of this manager should be passed to the common service via the context object as follows: context.setProperty(ISessionManager.CONTEXT_SESSION_MANAGER, MySessionManager.getInstance()); context.setProperty(ISessionManager.CONTEXT_SESSION_ID, "MySessionID"); Notice that a singleton of an implementation of the ISessionManager is passed in. The function of this singleton is to manage the creation of the resource sets across multiple sessions. Also notice that a session id is passed in to the context object to identify the session that is currently being processed. Locale ============ Each session may contain define a different locale. For example one session may process information in french while another session may process information in chinese. The Locale for a specific session can be passed in via the context object. This is showed as follows: context.setProperty(IImportHandler.CONTEXT_LOCALE, Locale.FRENCH); Note that each service provides a property name that signifies the Locale. For example the import service defines the IImportHandler.CONTEXT_LOCALE property name, while the export service defines the IExportHandler.CONTEXT_LOCALE, etc. As of TPTP 4.6.0, TPTP is in maintenance mode and focusing on improving quality by resolving relevant enhancements/defects and increasing test coverage through test creation, automation, Build Verification Tests (BVTs), and expanded run-time execution. As part of the TPTP Bugzilla housecleaning process (see http://wiki.eclipse.org/Bugzilla_Housecleaning_Processes), this enhancement/defect is verified/closed by the Project Lead since this enhancement/defect has been resolved and unverified for more than 1 year and considered to be fixed. If this enhancement/defect is still unresolved and reproducible in the latest TPTP release (http://www.eclipse.org/tptp/home/downloads/), please re-open. |