Quantcast

Proposal: javax.naming.spi.NamingManager.clearInitialContextFactoryBuilder()

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Proposal: javax.naming.spi.NamingManager.clearInitialContextFactoryBuilder()

Andrew Guibert


Hi all,

I've been cleaning up code that I work on to be Java 9 compatible, but have
ran into a blocker due to limitations of the Naming API.  I could work
around the blocker with an --add-opens, but I'd rather find a proper
solution.

*** Background
Currently the javax.naming.spi.NamingManager allows for setting a JVM-wide
InitialContextFactoryBuilder (ICFB) once and only once by using the static
NamingManager.setInitialContextFactoryBuilder() method [1].  The javadoc
clearly states:
> Once installed, the builder cannot be replaced.
If the set ICFB method is called after a ICFB is set, it will throw an
IllegalStateException.

This seems to be an overly-aggressive restriction of the API.  Currently my
product has code that reflects into the NamingManager class in order to
clear out the ICFB field using reflection.  To be Java 9 compliant, we
would like to remove this usage of deep reflection, but there are no clear
alternatives for doing so.

*** Use case
We have a JNDI server that may be started/stopped by other java code in the
same JVM.  If users stop the JNDI server we clear out the ICFB (using
reflection) when the JNDI server OSGi bundle deactivates, so that if a user
decides to start the JNDI server again in the same JVM, they may do so and
we simply set the ICFB when the JNDI server bundle activates.

*** Proposed Solution:
I could add a layer of indirection by creating an ICFB wrapper that
supports re-setting what ICFB it wraps.  However, this would cause the ICFB
wrapper and all other classes loaded using that bundle's classloader to be
leaked (since it can't be fully deactivated and garbage collected).  To
sort of work around this classloader leak, we would need to load the ICFB
wrapper using its own classloader.  Although the wrapper classloader would
still be leaked, it would have minimal impact because only the one wrapper
class would be leaked.

I am not sure why the "no resetting" restriction is on the NamingManager
API in the first place.  Is anyone aware why the API has this restriction?
In any case, the solution outlined above seems rather messy (as it only
solves the problem by mitigating a classloader leak), so I would like to
propose the following addition to the NamingManager API:

/**
 * Clears the InitialContextFactoryBuilder that was previously installed,
if any, so that a different one may be set at a later time.
 * @throws SecurityException - the builder cannot be cleared for security
reasons.
 * @see SecurityManager.checkSetFactory()
 */
public static void clearInitialContextFactoryBuilder()


Additionally, I think it would make sense to permit clearing the
NamingManager's ObjectFactoryBuilder using a similar mechanism.  Although I
don't have any immediate need or use case for this API, I can imagine
others may need the same.

[1]
http://download.java.net/java/jdk9/docs/api/javax/naming/spi/NamingManager.html#setInitialContextFactoryBuilder-javax.naming.spi.InitialContextFactoryBuilder-

- Andy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Proposal: javax.naming.spi.NamingManager.clearInitialContextFactoryBuilder()

Alan Bateman
On 08/05/2017 23:27, Andrew Guibert wrote:

> :
>
> I am not sure why the "no resetting" restriction is on the NamingManager
> API in the first place.  Is anyone aware why the API has this restriction?
> In any case, the solution outlined above seems rather messy (as it only
> solves the problem by mitigating a classloader leak), so I would like to
> propose the following addition to the NamingManager API:
>
>
I checked with one of the engineers that worked on the JNDI API a long
time ago and there doesn't seem to be any significant reason to disallow
it be changed.

Looking at it now then it might be simpler to change
setInitialContextFactoryBuilder to allow a new builder to be set rather
than introducing a clearXXX method. I can't imagine anything but tests
relying on IllegalStateException to be thrown if already set. A variant
to explore is setting the builder to a placeholder InitialContextFactory
rather than null.

So is your plan to proposal a patch and tests for this?

-Alan
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Proposal: javax.naming.spi.NamingManager.clearInitialContextFactoryBuilder()

Andrew Guibert

> From: Alan Bateman <[hidden email]>
> To: Andrew Guibert <[hidden email]>, [hidden email]
> Date: 05/10/2017 07:32 AM
> Subject: Re: Proposal:
> javax.naming.spi.NamingManager.clearInitialContextFactoryBuilder()
>
> On 08/05/2017 23:27, Andrew Guibert wrote:
>
> > :
> >
> > I am not sure why the "no resetting" restriction is on the
NamingManager
> > API in the first place.  Is anyone aware why the API has this
restriction?
> > In any case, the solution outlined above seems rather messy (as it only
> > solves the problem by mitigating a classloader leak), so I would like
to

> > propose the following addition to the NamingManager API:
> >
> >
> I checked with one of the engineers that worked on the JNDI API a long
> time ago and there doesn't seem to be any significant reason to disallow
> it be changed.
>
> Looking at it now then it might be simpler to change
> setInitialContextFactoryBuilder to allow a new builder to be set rather
> than introducing a clearXXX method. I can't imagine anything but tests
> relying on IllegalStateException to be thrown if already set. A variant
> to explore is setting the builder to a placeholder InitialContextFactory
> rather than null.

Thank you for checking on this Alan!

I agree relaxing the setXXX method would be cleaner than introducing a new
clearXXX method, but I wasn't sure what the policy was on behavior changes
to existing APIs.  But I think you are right, there probably isn't any
"real" code that depends on this behavior (aside from code like mine that
forcibly clears if an ISE is thrown).

> So is your plan to proposal a patch and tests for this?

I need to check with my management to get all the legal approvals before
proposing a patch, so it might take a while.  Since it's likely a very
small code change I won't be offended if someone else proposes the patch
for me =)
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Proposal: javax.naming.spi.NamingManager.clearInitialContextFactoryBuilder()

Alan Bateman
On 10/05/2017 14:33, Andrew Guibert wrote:

> :
>
> I need to check with my management to get all the legal approvals
> before proposing a patch, so it might take a while.  Since it's likely
> a very small code change I won't be offended if someone else proposes
> the patch for me =)
>
I don't see too many people interested in JNDI these days so I think
it's all yours.

-Alan
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Proposal: javax.naming.spi.NamingManager.clearInitialContextFactoryBuilder()

Andrew Guibert
In reply to this post by Alan Bateman
Alan, could you please commit this patch for me?  We've tested it locally
and all of the jdk_other tests pass with this change on jdk9.

93d92
<      * Once installed, the builder cannot be replaced.
101d99
<      * @exception IllegalStateException If a factory has already been
installed.
109,111d106
<         if (object_factory_builder != null)
<             throw new IllegalStateException("ObjectFactoryBuilder already
set");
<
739,740c734
<      * the security manager to do so. Once installed, the builder cannot
<      * be replaced.
---
>      * the security manager to do so.
747d740
<      * @exception IllegalStateException If a builder was previous
installed.
754,757d746
<             if (initctx_factory_builder != null)
<                 throw new IllegalStateException(
<                     "InitialContextFactoryBuilder already set");
<

Alan Bateman <[hidden email]> wrote on 05/10/2017 07:32:47 AM:

> From: Alan Bateman <[hidden email]>
> To: Andrew Guibert <[hidden email]>, [hidden email]
> Date: 05/10/2017 07:32 AM
> Subject: Re: Proposal:
> javax.naming.spi.NamingManager.clearInitialContextFactoryBuilder()
>
> On 08/05/2017 23:27, Andrew Guibert wrote:
>
> > :
> >
> > I am not sure why the "no resetting" restriction is on the
NamingManager
> > API in the first place.  Is anyone aware why the API has this
restriction?
> > In any case, the solution outlined above seems rather messy (as it only
> > solves the problem by mitigating a classloader leak), so I would like
to

> > propose the following addition to the NamingManager API:
> >
> >
> I checked with one of the engineers that worked on the JNDI API a long
> time ago and there doesn't seem to be any significant reason to disallow
> it be changed.
>
> Looking at it now then it might be simpler to change
> setInitialContextFactoryBuilder to allow a new builder to be set rather
> than introducing a clearXXX method. I can't imagine anything but tests
> relying on IllegalStateException to be thrown if already set. A variant
> to explore is setting the builder to a placeholder InitialContextFactory
> rather than null.
>
> So is your plan to proposal a patch and tests for this?
>
> -Alan
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Proposal: javax.naming.spi.NamingManager.clearInitialContextFactoryBuilder()

Alan Bateman
On 11/05/2017 22:25, Andrew Guibert wrote:

> Alan, could you please commit this patch for me? We've tested it
> locally and all of the jdk_other tests pass with this change on jdk9.
>
> 93d92
> < * Once installed, the builder cannot be replaced.
> 101d99
> < * @exception IllegalStateException If a factory has already been
> installed.
> 109,111d106
> < if (object_factory_builder != null)
> < throw new IllegalStateException("ObjectFactoryBuilder already set");
> <
> 739,740c734
> < * the security manager to do so. Once installed, the builder cannot
> < * be replaced.
> ---
> > * the security manager to do so.
> 747d740
> < * @exception IllegalStateException If a builder was previous installed.
> 754,757d746
> < if (initctx_factory_builder != null)
> < throw new IllegalStateException(
> < "InitialContextFactoryBuilder already set");
> <
>
I assume you will be able to get an IBM contributor to sponsor this. It
will need an issue in JIRA and tests. All API changes for Java SE 10 and
beyond will be reviewed by the CSR group [1], that process will be
opening soon.

-Alan

[1] https://wiki.openjdk.java.net/display/csr/Main
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Proposal: javax.naming.spi.NamingManager.clearInitialContextFactoryBuilder()

Andrew Guibert

> From: Alan Bateman <[hidden email]>
> To: Andrew Guibert <[hidden email]>
> Cc: [hidden email]
> Date: 05/12/2017 04:02 AM
> Subject: Re: Proposal:
> javax.naming.spi.NamingManager.clearInitialContextFactoryBuilder()
>
> On 11/05/2017 22:25, Andrew Guibert wrote:
> Alan, could you please commit this patch for me? We've tested it
> locally and all of the jdk_other tests pass with this change on jdk9.
>
> 93d92
> < * Once installed, the builder cannot be replaced.
> 101d99
> < * @exception IllegalStateException If a factory has already been
installed.

> 109,111d106
> < if (object_factory_builder != null)
> < throw new IllegalStateException("ObjectFactoryBuilder already set");
> <
> 739,740c734
> < * the security manager to do so. Once installed, the builder cannot
> < * be replaced.
> ---
> > * the security manager to do so.
> 747d740
> < * @exception IllegalStateException If a builder was previous installed.
> 754,757d746
> < if (initctx_factory_builder != null)
> < throw new IllegalStateException(
> < "InitialContextFactoryBuilder already set");
> <
> I assume you will be able to get an IBM contributor to sponsor this.
> It will need an issue in JIRA and tests. All API changes for Java SE
> 10 and beyond will be reviewed by the CSR group [1], that process
> will be opening soon.
>
> -Alan
>
> [1] https://wiki.openjdk.java.net/display/csr/Main

Hi Alan, I've checked within IBM and it appears we do not have any OpenJDK
committers.  Still trying to chase down if we have anyone in IBM who can
drive this commit.  Do you know of anyone?

Also, you mentioned adding this API change to Java SE *10*.  Are we past
the cutoff for making changes in JDK 9?  I was hoping this (and a handful
of other proposals I have brewing) could be made for JDK 9.  If not, we're
going to be stuck using the JVM options for the lifetime of JDK 9.
Loading...