persistence@glassfish.java.net

Re: Code review for: [Issue 1107] Logger defined by a property doesn't work]

From: Wonseok Kim <guruwons_at_gmail.com>
Date: Fri, 13 Oct 2006 04:39:44 +0900

Marina,
You're correct. JavaLog.clone() is not doing deep copy now.
AbstractSessionLog.setSessionLog() calls clone() and setSession() not to
share SessionLog instance, but it doesn't behave correctly for JavaLog as
you mentioned.

So there are two options.
1. Leave code as it is
2. Modify JavaLog to behave correctly - do deep copy in clone() or create
new HashMap instances in setSession().

For now just do 1. 2 is an independent issue and needs more work. :-)

Thanks
- Wonseok

On 10/13/06, Marina Vatkina <Marina.Vatkina_at_sun.com> wrote:
>
> Hi Wonseok,
>
> Yes, I was also thinking about that, but clone() is a shallow copy, and
> the 2 loggers will share all the maps defined in JavaLog between them,
> which can cause a memory leak as when the PU is unloaded, it's loggers
> will still be registered in the maps.
>
> What do you think?
>
> thanks,
> -marina
>
> Wonseok Kim wrote:
> > Hi Marina,
> >
> > This is another issue but could you modify updateLoggers() also?
> > I found that we don't need to instantiate two SessionLog instances
> > because Session.setSessionLog() does clone its parameter always.
> >
> > The following is modified code:
> >
> > protected void updateLoggers(AbstractSession session, Map m,
> > ServerPlatform serverPlatform){
> > // Logger(SessionLog type) can be specified by the logger
> > property or ServerPlatform.getServerLog().
> > // The logger property has a higher priority to
> > ServerPlatform.getServerLog().
> >
> > String loggerClassName =
> > PropertiesHandler.getPropertyValueLogDebug(
> TopLinkProperties.LOGGING_LOGGER,
> > m, session);
> > Class sessionLogClass = null;
> > if (loggerClassName != null) {
> > sessionLogClass = findClassForProperty(loggerClassName,
> > TopLinkProperties.LOGGING_LOGGER);
> > }
> >
> > SessionLog sessionLog = null;
> > if(sessionLogClass != null){
> > try {
> > sessionLog = (SessionLog)sessionLogClass.newInstance();
> > } catch (Exception ex) {
> > throw
> > EntityManagerSetupException.failedToInstantiateLogger (loggerClassName,
> > TopLinkProperties.LOGGING_LOGGER, ex);
> > }
> > } else if(serverPlatform != null){
> > // if ServerPlatform.getServerLog () returns null, we won't
> > set loggers
> > sessionLog = serverPlatform.getServerLog();
> > }
> >
> > // Set loggers for the singleton logger and the session logger.
> > // Don't change default loggers if null
> > if (sessionLog != null){
> > AbstractSessionLog.setLog(sessionLog);
> > session.setSessionLog (sessionLog); // sessionLog instance
> > won't be shared because it's cloned internally.
> > }
> >
> > //Bug5389828. Update the logging settings for the singleton
> > logger.
> > initOrUpdateLogging(true, m, AbstractSessionLog.getLog());
> > initOrUpdateLogging(true, m, session.getSessionLog());
> > }
> >
> > Thanks
> > - Wonseok
> >
>