Bug 51744 - JNDI Lookup Error after a Context is closed
Summary: JNDI Lookup Error after a Context is closed
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 7.0.22
Hardware: PC All
: P2 critical (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
: 54168 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-08-30 08:43 UTC by Anjana Fernando
Modified: 2015-09-24 01:48 UTC (History)
4 users (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Anjana Fernando 2011-08-30 08:43:40 UTC
I was integrating Atomikos into Tomcat to register the transaction manager, and the lookup for the transaction manager works fine in JNDI, unless someone else, looks up a JNDI context and closes it. And subsequent calls to lookup the transaction manager also fails saying java:comp is not found. I went through the code and found the problem at "org.apache.naming.SelectorContext". When methods of that class is calls, those are delegated to a Context received through "getBoundContext()", which returns a "org.apache.naming.NamingContext" object. And these are stored in a map in ContextBindings class. So the getBoundContext checks this cache, and if it's found it returns it or else, create a new object and store it and returns it. So the problem happens, if someone calls "close()" in the NamingContext object. Then it's implemented in the following way,

   public void close()
        throws NamingException {
        env.clear();
    }

Which just clears the environment, but it doesn't in no way invalidate that cache in the ContextBindings class to remove its entry. So in the next time also when getBoundContext is called to get the same context, it will return the earlier 'closed' context without creating a new one. And the environment it has would be cleared and subsequent lookups done from that will fail. 

So I guess the proper fix would be to remove that context from ContextBinding's cache when its close method is closed.

Cheers,
Anjana.
Comment 1 Mark Thomas 2011-08-31 14:20:15 UTC
Given that the JNDI context is meant to be (largely) read-only, I don't think calls to close() should be permitted here. I have added a call to checkWriteable() before the environment is cleared.

This fix has been applied to trunk and 7.0.x and will be included in 7.0.22 onwards.
Comment 2 Anjana Fernando 2011-08-31 14:33:31 UTC
Hi Mark,

So for the situation I'm facing, in the close method, if the environment is cleared, will this be removed from "ContextBindings" also? .. Since that was the problem I faced. 

Cheers,
Anjana.
Comment 3 Mark Thomas 2011-08-31 14:35:00 UTC
No. The attempt to use the close() method will fail.
Comment 4 Anjana Fernando 2011-08-31 14:43:14 UTC
Hi Mark,

Sorry, maybe I misunderstood, I guessed the modified version would be something like the following,

public void close() throws NamingException {
    if (checkWriteable()) {
        env.clear();
    } else {
        throw something...
    }
}

so in the case of checkWriteable() == true, the environment is cleared and if the user again looks up that context, wouldn't my same problem occur again? .. 

Cheers,
Anjana.
Comment 5 Mark Thomas 2011-08-31 14:48:59 UTC
Rather than guessing why don't you look at the source code and see what it actually does? You might also want to investigate when the JNDI context is writeable.
Comment 6 Anjana Fernando 2011-08-31 15:32:13 UTC
Hi Mark,

OK, sorry, I should have looked at the code before. I just didn't know where the svn for this was (yeah it was a simple looking around the tomcat project page), I thought you wouldn't mind showing the code. 

Anyways, yeah, the code was close to what I assumed. So a quick question, let's assume the context is writeable (I'm still not that sure when they are exactly writeable or not), just bare with me, so in the case where "checkWriteable" doesn't throw anything, wouldn't you want to clear this object from the map at ContextBindings?, so later the method "getBoundContext" from SelectorContext wouldn't return an invalid Context object, and also I guess you would want to remove it from that map for it to be GC'ed later. 

Cheers,
Anjana.
Comment 7 Mark Thomas 2011-08-31 15:37:20 UTC
I'll repeat: "You might also want to investigate when the JNDI context is
writeable."

I really do not appreciate code reviews based on guess work. If you are going to comment on the code please have the courtesy to read and understand the relevant portions of the source rather than guessing what the source code might be.

I do not intend replying to further comments on this issue until you have actually read the relevant code.
Comment 8 Anjana Fernando 2011-08-31 15:57:33 UTC
Mark,

You could have used all those words to give me a simple explanation, I gave the comments and reported the bug to my ability, I explained the issue as I understood it, and since I actually faced it, it's not like I "guessed" this issue or imagined it and reported it. And I accept I'm not in any kind of an expert in these areas, that's why we talk to you guys, the actual developers, to get some help. Talking about "courtesy", you should have the common courtesy to give a proper response or at-least direct the user to some resources to understand it more. So much for the spirit of open source development! .. I don't expect you to respond to this also, good bye!.

Cheers,
Anjana.
Comment 9 Leonid Mikhailov 2011-10-25 21:17:49 UTC
Guys,

It appears that something is not quite right with this fix in 7.0.22. The following worked just fine in 7.0.14 (and GlassFish, WebLogic, and WebSphere) and now fails on envCtx.close() with "Context is read only" message.

javax.naming.Context initCtx = new InitialContext();
javax.naming.Context envCtx = (javax.naming.Context) initCtx.lookup("java:comp/env");
//some clever logic that uses envCtx
envCtx.close();//Fails here!
initCtx.close();

According to JavaDoc I should be able to safely close the context when I am done with it.  

Thanks,

Leon
Comment 10 Konstantin Kolinko 2011-10-26 11:01:34 UTC
Reopening, so that new issue in comment 9 is not lost.
Do you have a stack trace?
Comment 11 Leonid Mikhailov 2011-10-26 21:24:19 UTC
Here you go:

javax.naming.NamingException: Context is read only
	at org.apache.naming.NamingContext.checkWritable(NamingContext.java:941)
	at org.apache.naming.NamingContext.close(NamingContext.java:747)
	at com.esri.sds.rest.AdminDatasourcesResource.listDatasources(AdminDatasourcesResource.java:44)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
	at java.lang.reflect.Method.invoke(Unknown Source)
	at com.sun.jersey.server.impl.model.method.dispatch.AbstractResourceMethodDispatchProvider$TypeOutInvoker._dispatch(AbstractResourceMethodDispatchProvider.java:149)
	at com.sun.jersey.server.impl.model.method.dispatch.ResourceJavaMethodDispatcher.dispatch(ResourceJavaMethodDispatcher.java:67)
	at com.sun.jersey.server.impl.uri.rules.HttpMethodRule.accept(HttpMethodRule.java:259)
	at com.sun.jersey.server.impl.uri.rules.ResourceClassRule.accept(ResourceClassRule.java:83)
	at com.sun.jersey.server.impl.uri.rules.RightHandPathRule.accept(RightHandPathRule.java:133)
	at com.sun.jersey.server.impl.uri.rules.RootResourceClassesRule.accept(RootResourceClassesRule.java:71)
	at com.sun.jersey.server.impl.application.WebApplicationImpl._handleRequest(WebApplicationImpl.java:990)
	at com.sun.jersey.server.impl.application.WebApplicationImpl.handleRequest(WebApplicationImpl.java:941)
	at com.sun.jersey.server.impl.application.WebApplicationImpl.handleRequest(WebApplicationImpl.java:932)
	at com.sun.jersey.spi.container.servlet.WebComponent.service(WebComponent.java:384)
	at com.sun.jersey.spi.container.servlet.ServletContainer.service(ServletContainer.java:451)
	at com.sun.jersey.spi.container.servlet.ServletContainer.doFilter(ServletContainer.java:797)
	at com.sun.jersey.spi.container.servlet.ServletContainer.doFilter(ServletContainer.java:770)
	at com.sun.jersey.spi.container.servlet.ServletContainer.doFilter(ServletContainer.java:731)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:243)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:210)
	at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:224)
	at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:169)
	at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:472)
	at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:168)
	at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:100)
	at org.apache.catalina.valves.AccessLogValve.invoke(AccessLogValve.java:929)
	at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:118)
	at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:405)
	at org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:964)
	at org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:515)
	at org.apache.tomcat.util.net.JIoEndpoint$SocketProcessor.run(JIoEndpoint.java:302)
	at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
	at java.lang.Thread.run(Unknown Source)

Thanks,

Leon
Comment 12 Mark Thomas 2011-10-27 16:11:07 UTC
To quote from section EE.5.3.4 of the Java EE spec
<quote>
The container must ensure that the application component instances have only read access to their naming context. The container must throw the javax.naming.OperationNotSupportedException from all the methods of the javax.naming.Context interface that modify the environment naming context and its subcontexts.
</quote>

I would argue that the close() method is a method that "modifies the environment naming context" and therefore an exception should be thrown here. Tomcat is, however, not throwing the right exception in this case. Fortunately, the exception we should be throwing is a sub-class of the one we are throwing so changing it shouldn't break anything.

Providing an option on the StandardContext to silently swallow this error rather than throwing an Exception looks doable. I'll take a look.
Comment 13 Mark Thomas 2011-10-28 14:33:29 UTC
The correct exception is now thrown and the option to control whether or not an exception is thrown has been added to the StandardContext.

Both of these changes have been made to trunk and 7.0.x and will be included in 7.0.23 onwards.
Comment 14 Leonid Mikhailov 2011-12-01 23:17:40 UTC
(In reply to comment #13)
> The correct exception is now thrown and the option to control whether or not an
> exception is thrown has been added to the StandardContext.
> 
> Both of these changes have been made to trunk and 7.0.x and will be included in
> 7.0.23 onwards.

Mark,

I just installed 7.0.23 and I still see "Context is read only" exception thrown. 

In your previous message you mentioned:
> To quote from section EE.5.3.4 of the Java EE spec
> <quote>
> The container must ensure that the application component instances have only
> read access to their naming context. The container must throw the
> javax.naming.OperationNotSupportedException from all the methods of the
> javax.naming.Context interface that modify the environment naming context and
> its subcontexts.
> </quote>
> 
> I would argue that the close() method is a method that "modifies the
> environment naming context" and therefore an exception should be thrown here.

I hoped to avoid an argument but... I believe you are not interpreting the spec correctly. Here is a recommendation from a tutorial (http://docs.oracle.com/javase/jndi/tutorial/ldap/connect/close.html) explaining how one is supposed to work with Contexts:

"Normal garbage collection takes care of removing Context instances when they are no longer in use. Connections used by Context instances being garbage collected will be closed automatically. Therefore, you do not need to explicitly close connections. Network connections, however, are limited resources and for certain programs, you might want to have control over their proliferation and usage." 

And here is a promoted usage pattern:
    // Create initial context
    DirContext ctx = new InitialDirContext(env);
    // Get a copy of the same context
    Context ctx2 = (Context)ctx.lookup("");
    // Get a child context
    Context ctx3 = (Context) ctx.lookup("ou=NewHires");
    // do something useful with ctx, ctx2, ctx3
    // Close the contexts when we're done
    ctx.close();
    ctx2.close();
    ctx3.close();

Closing a context has nothing to do with modifying it - you are just telling the system that you are done with a resource and it can be safely released at this time. Modifying a context means adding and/or deleting something from it. 

I have written a sample application that works as described above with no logged exceptions in GlassFish and in all Tomcat versions prior to 7.0.22. 
I can send it to you if you like.

I don't believe the current implementation is correct. No exceptions should be logged by default.

Regards,

Leon
Comment 15 mmaurer61 2012-03-27 22:03:48 UTC
I'm using Tomcat 7.0.25 and am still seeing this same issue. Any attempt to close a Context object results in the exception "Context is read only", and I am unable to instantiate a JNDI DataSource. My code worked fine on previous versions (earlier than 7.0.22), but is now broken.

Are we supposed to just leave the Context open, and it will not cause memory leaks?
Comment 16 ric.almeida 2012-06-21 12:44:12 UTC
(In reply to comment #14)

> I hoped to avoid an argument but... I believe you are not interpreting the
> spec correctly. Here is a recommendation from a tutorial
> (http://docs.oracle.com/javase/jndi/tutorial/ldap/connect/close.html)

I agree and I also hope to avoid an argument but I also hope that the bug gets fixed... Just retried on 7.0.27 and the current behaviour is incorrect.
Comment 17 Mark Thomas 2012-06-21 18:56:38 UTC
(In reply to comment #16)
> I agree and I also hope to avoid an argument but I also hope that the bug
> gets fixed... Just retried on 7.0.27 and the current behaviour is incorrect.

You're not going to get an argument but you are going to be told to have the courtesy to actually read the bug before re-opening it.

To repeat:
- the current behaviour is required by the specification
- if you don't like it (and there are plenty of valid reasons why you might not) there is an option available on the StandardContext to disable it.

The only reason I can think of that would warrant re-opening this bug is if the option on the StandardContext did not work but that is not what you are saying.
Comment 18 Mark Thomas 2012-11-19 20:54:43 UTC
*** Bug 54168 has been marked as a duplicate of this bug. ***
Comment 19 bingalee7 2015-09-24 01:48:48 UTC
The work-around is to apply jndiExceptionOnFailedWrite to the context (https://tomcat.apache.org/tomcat-7.0-doc/config/context.html#Standard_Implementation), e.g.

<Context jndiExceptionOnFailedWrite="false">