OpenMRS Trunk

We cannot move along with reporting until this bug is fixed. The unit test is committed in the reporting module (it happens with a class specific to the reporting module). Let me know if you need more information.

Some random thoughts:

We have an extra layer (interface Persister) between the service layer and the DAO that might be adding some complication here. The Persister handlers know how to persist certain reporting objects (they can save, purge, etc). For instance, the SerializedCohortDefinitionPerister uses the SerializedObjectDAO (it just delegates in all/most cases). So calling CohortDefinitionService.saveCohortDefinition(CohortDefinition cd) will find the proper persister and call that persister's saveCohortDefinition() method which then delegates to the DAO. Does the Perister need the @Transactional annotation in order to participate in the current transaction? In other words, if the Service which is annotated with @Transactional delegates to the Persister, then I assume we probably need @Transactional around all of the Persister methods, right?

I think I figured this out. While I was debugging this the other day, I ran into another bug (see the stacktrace contained in the file stacktrace-APIException-no-handler-for-class.txt). I added @Transactional to CohortDefinitionPersister, since we mostly likely want all of the persister's methods to be enclosed in a transaction (read-only or read-write). Adding the @Transactional to the interface caused the HandlerUtil to have trouble finding the appropriate persister for the PatientCharacteristicCohortDefinition. It should have found, but the HandlerUtil probably short circuits when it finds the @Transactional annotation instead of the @Handler annotation. I haven't looked at the HandlerUtil code yet.

So I just added the @Transactional to the specific implementation class (SerializedCohortDefinitionPersister) and it appears to be working as expected. I'm going to play around with this a little more, but I'm cool with that as a solution. However, I really think we should be able to put the @Transactional on all Persister interfaces, instead of specific implementations. So this no longer appears to be a serialization issue ... it looks like it could be a Handler issue.

Does everyone agree with that?

Looks like HandlerUtil has this:

Handler handlerAnnotation = handler.getClass().getAnnotation(Handler.class);

so it is not short circuiting anything.

I debugged this issue a little further today/tonight. I'm back to the theory that this is a hibernate/serialization issue (not a persister/spring issue), but I can't pinpoint exactly what's going on. I ran the save cohort definition use case through the debugger and could not figure out what is actually causing the LazyInitializationException (the session is open). What I do know is that there's a reference to a User object that is throwing the error while the serialization code is putting together the XML to serialize.

I decided to remove the CohortDefinitionPersister from the picture and delegated save() calls straight from the BaseCohortDefinitionService.saveCohortDefinition() to SerializedObjectDAO.saveObject(). This still produced the error, so I think that means that this is either a serialization or Hibernate issue.

I'm going to try to put together a valid test case within trunk, so we can test it there. Otherwise, anyone who wants to look into this, just download the reporting module and run the CohortDefinitionTest.

Note: I cannot move forward with reporting until this is fixed, so I need some help with this.

I was starting to wonder if this might be due to the fact that the "authenticated user" that is used to populate the creator/changedBy attributes is fetched in a different session. So I tried the following code and was able to get a little further.

public CohortDefinition saveCohortDefinition(CohortDefinition definition) throws APIException \{
   User user = Context.getUserService().getUserByUsername("jmiranda");
   definition.setCreator(user);
   definition.setChangedBy(user);		
   return dao.saveObject(definition);

Thoughts?

Sorry, jmiranda. The serialization framework has let you unable to move forward with reporting many days. Its my fault :-/

I has seen the stacktrace. I don't know why the session is closed?

In method "CustomObjectIdDictionary.lookupId(Object obj)", it will call obj.equals() to compare with every object which has already been serialized before obj, so that we will just add an "id" attribute for obj in the serialized xml string if here is an object equaling with obj and it has been serialized before obj.

So, i think the key problem is that we should know why the session is closed when we call "org.openmrs.User$$EnhancerByCGLIB$$b964cda5.equals()"?

I will learn to download your module to try unit test. I will try my best.

Thanks. Can you try to reproduce the error in a unit test in trunk or serialization?

I think the session is "closed" when trying to work with the user.userProperties attribute. I don't think the session is actually closed, just that Hibernate doesn't seem to have a reference to the user properties map. This is the Hibernate code that triggers the exception to be thrown (it returns false). I assume it's the last statement (session.getPersistenceContext().containsCollection(this)) that causes the method to return false.

org.hibernate.collection.AbstractPersistentCollection.java(173)

private final boolean isConnectedToSession() \{
		return session!=null && 
				session.isOpen() &&
				session.getPersistenceContext().containsCollection(this);
	\}

Do you mean the reason is that Hibernate does have a reference to user.userProperties? I think i am not very clear your meaning.

But you can see the unit test "UserSerializaionTest" in pacakge "org.openmrs.serialization.xstream" of trunk. And in the unit test, i have get a user through Hibernate and its attribute "userProperties" is serialized OK. So i think maybe i haven't very clearly understand your meaning.

Yep, i also want to reproduce this error in trunk or serialization, but sorry, i didn't know how i can reproduce this error? Can you give me a few suggestion?

Thanks, jmiranda

partial serialized xml for patient characteristic cohort definition

The attached file shows the partial output of the serialized version of a PatientCharacteristCohortDefinition that I "watched" during a debugging session. As you can see, most of the XML is "user" information.

Since full "user" information is almost never needed, can we agree to use the "short" or "medium" serializer for the User object by default?

How do we enable "short" or "medium" serialization?

jmiranda, why this time you can serialize "PatientCharacteristicCohortDefinition" ok? How you can do it?

Yes, i have complete a short serializer, it will only serialize uuid of User. But now this short serializer is just in serialization branch and haven't been merged into trunk. I think it will be added into trunk, soon.

If you want enable "short" serializer, there are two manners.

!#1 While serialize a user object, just call like this:

User user = Context.getUserService().getUser(1);
String xmlOutput = Context.getSerializationService().serialize(user, XStreamShortSerializer.class);

!#2 You can enable short serializer in Spring's config file "applicationContext-service.xml"

You can see "serialization" branch and we let bean "serializationServiceTarget"'s property "defaultSerializer" point to "xstreamShortSerializer" will enable short serialization always.

jmiranda, i have download reporting module and run the unit test "CohortDefinitionTest".

It throw an exception and i have attached the stack trace in "stacktrace-PropertyValueException.txt".

And after i added a statement "cohortDefinition.setName("Testing")", the test method "shouldSaveCohortDefinition()" run ok.

So it seems i can not reproduce the error you have found. Can you write a unit test which will reproduce that error, i think it will help me to find whether the serialization framework has bug.

Ok, so this is one of those bugs that only happens on my machine. Sweet. Could one of you guys (Ben, Mike, Darius) run the test to see if you get the LazyInitializationException?

I just added a second test class to the reporting module CohortDefinitionServiceTest that attempts to save a cohort definition through the CohortDefinitionService and then directly through the SerializedObjectDAO. It's able to save when calling the DAO directly, but not when it goes through the Cohort Definition Service/Persister. Will get back to debugging this later today/tonight.

Thanks so much for all your help Lu.

No thanks, jmiranda.

It's my pleasure to cooperate with you to resolve this bug.

Hi All,

I checked out the reporting-trunk module and ran the CohortDefinitionServiceTest, and I get the same exception that Justin does.

Lu, it ran fine for you? Ben, can you check this out and try it and see if you hit the error or not?

I'm running Java 1.6.0.13 on Ubuntu, with 1.5-compliance enabled.

Removed stacktrace for readability.  See attached stacktrace "stacktrace-1604-djazayeri.txt".

-Darius

yep, i runned the CohortDefinitionServiceTest and i also get such an exception. I am not very clear, why using the second method with DAO is ok, but with Service is wrong.

Sorry, i am not familiarize Session's cycle of Hibernate. Why we through service and service call persister to call dao, then throw an exception? Anyone have idea?

Hi, All

I have modified a few logic in Serialization Framework. I runned "CohortDefinitionTest" in my local machine agian and that test case doesn't throw org.hibernate.LazyInitializationException now.

So, please wait a few time, i will talk about with my Mentor Ben. Hope to merge this new modification to trunk 1.5 as soon as possible.

Oh, sorry, everyone

Althrough the org.hibernate.LazyInitializationException disappeared, but the serialized xml string is not same as previous.

So, i think maybe i remain need to think another resolution :-/

Can anyone help me to answer following questions?

!#1 What dose @Transactional mean?

!#2 Does @Transactional have relationship with Hibernate's Session?

Lu - The new attachment is to show you that there seems to be a bug in the how the "creator" attribute is being serialized. Shouldn't we be converting that back to class="org.openmrs.User"?

<org.openmrs.module.cohort.definition.PatientCharacteristicCohortDefinition>
  <uuid>b9e8f1fc-f6c7-416c-b828-4ff1e557f17a</uuid>
  <name>test1</name>
  <description></description>
  <creator class="org.openmrs.User$$EnhancerByCGLIB$$fff1cb7d">jmiranda</creator>
  <dateCreated>2009-06-30 03:19:10.967 CDT</dateCreated>
  <changedBy>jmiranda</changedBy>
...

@Transaction is an annotation that tells spring to wrap methods of the annotated class (for instance, PatientService) with a transaction. Spring creates dynamic proxies for our Service layer classes and uses AOP to wrap each method with a transaction using the appropriate propagation type specified by the annotation's 'propogation' attribute. The 'propagation' attribute defaults to REQUIRED which means it will use the existing transaction or create a new transaction if one does not exist.

I had a hunch that the Service and Persister both needed to declare the @Transactional annotation. However, the Persister cannot (at least not without a rewrite of the Handler code) declare the @Transaction because it breaks HandlerUtil.getPreferredHandler() method due to the fact that the Persister becomes a dynamic proxy (i.e. $Proxy58), so it seems almost impossible to inspect the proxy object to find out whether it's a handler and what type of objects it handles.

Hope that makes a little bit of sense.

Yes, jmiranda, you are right.

In fact, in openmrs 1.5 branch, its serialization mechanism is xstream's default mechanism. This mechanism is now you saw, it add attribute "class=org.openmrs.User$$EnhancerByCGLIB$$fff1cb7d".

And in serialization branch and Openmrs1.5 trunk, the serialization mechanism is designed by own, it will first initialize a cglib proxy to real object. and then serialize that real object, not the cglib proxy. So that the attribute "class"'s value will be correct as "org.openmrs.User".

But, now as LazyInitializationException occurred in reporting module, in xstream serializer it can not get the matching Hibernate session to initialize cglib proxy.

I saw a few topics, the reason why here is a LazyInitializationException maybe is that the old hibernate session which holds those cglib proxy has been closed while xstream serializer to serialize proxy.

Yep, i have tried to add @Transactional to Persister class, but failure.

I also think the problem is in Persister class, if we through Persister to call serialization method, the session will be closed? is it? jmiranda

I'm lost. Which module on which branch doesn't work?

1.5.x doesn't have Lu's serialization fixes yet. Trunk has the fixes. The serialization branch might have a few more fixes than trunk. (Is that true Lu?)

Yeah - I think we might need a new summary of the ongoing problems. Justin, didn't we find that some of our problems were due to using the wrong jars in our module, etc? And so we can close out any issues that were specifically related to that? I think right now we are not all on the same page with regard to the current problems.

Ben / Lu - we are specifically testing this out by running a module that uses serialization as it is present in 1.5.x - so we should focus our efforts on trying to get it working here first, and then worry about trunk. If you don't want to commit to 1.5.x, we can work off of patches, but please focus your effort on serialization support for 1.5.x.

Ideally, Justin would be creating failing unit tests in the reporting module, and you guys would check this out and ensure all tests pass for the latest 1.5.x code. Then we can avoid this back and forth a bit. Is this possible?

We do need to get serialization to a long term working state in the 1.5.x branch. I can't remember exactly why I didn't backport immediately, maybe because I was waiting on some feedback or more tests? I don't know. Either way, I would like to move the serialization stuff Lu did to 1.5.x so that we have a constant serialization base to build from. Does that sound reasonable?

Therefore, Justin should be trying these with trunk because at this exact moment, thats where the serialization stuff is until I move it from trunk to 1.5.x. If we determine that changes are needed, we can put them in the trunk/1.5.x/serialization branches.

Yeah, Ben

It's as same as you said. trunk has the fixes. And serialization branch has a few more fixes than trunk.

Now, the bug is in reporting module which based on Openmrs trunk 1.5.

I am trying my best to resolving this ticket.

Replying to [bwolfe|comment:24]:
> I'm lost. Which module on which branch doesn't work?
>
> 1.5.x doesn't have Lu's serialization fixes yet. Trunk has the fixes. The serialization branch might have a few more fixes than trunk. (Is that true Lu?)

Thanks, Ben

Yes, 1.5.x branch is not suitable to do serialization work on it now. Because we haven't move our serialization framework from trunk to 1.5.x branch.

Looks like Lu has made changes to the serialization branch around the same place where Justin's code is failing. I'll merge those to trunk and try the module again.

Lu: did you make test cases for those latest changes?

Oh, sorry, Ben

Do you mean changes in "CustomObjectIdDictionary"? Please don't merge them to trunk, sorry. Those changes are wrong, they are my first version modification and i find those changes doesn't work for this ticket. I will revert serialization branch to the previous version.

And i will continue find a correct resolution for this ticket. Sorry, i think my speed is too slow :-/

I really not very familiarize about hibernate's session machenism.

Replying to [bwolfe|comment:29]:
> Looks like Lu has made changes to the serialization branch around the same place where Justin's code is failing. I'll merge those to trunk and try the module again.
>
> Lu: did you make test cases for those latest changes?

The reporting module sort of works with the 1.5.x branch. The 1.5.x branch seems to use a shorter serialization strategy as I don't see creators of creators of creators in the serialized XML. What doesn't work in 1.5.x is documented in TRAC-1608. It leaves references to CGLIB proxies in the XML, so when you restart the server those classes are no longer in the classpath and the serializer throws a CannotResolveClassException.

The reporting module DOES NOT work with trunk. Contrary to Mike's comment, this ticket is still open and relevant. Trunk serialization seems to use an exploding strategy where it traverses the entire object graph and spits out every detail about an object. That means we get information about the user who created the user who created the user that modified Darius's user account. Whether that's right or wrong, I think we need to find the root cause of this because it migth pop up again.

While debugging this issue, I added an @Transactional annotation to the CohortDefinitionPersister in order to see if the LazyInitializationException was due to the transaction not being propagated through the Service->Persister->Dao. However, this lead to another issue dealing with dynamic proxies (see TRAC-1609).

Hi, All

I have a puzzled question, can anyone help me to answer this question.

Today, i have written a temp unit test in reporting module which is in my local machine. And in my machine, i let reporting module based on openmrs trunk 1.5.

After i tested that unit test. I find a very puzzled question, it is that when reporting module call Context.getSerializationService.serialize(...) to serialize a object, the xstream can successfully initialize some cglib proxy through hibernate session, but here are a few other cglib proxy which can not be initialized and throw such underlying exception as described in attachment "stacktrace of exception which is throwed while serializing.txt".

Can anyone tell me why in Context.getSerializationService.serialize(...), here are some proxy can be initialized, but some other proxy can not be initialized?

Note: this question is only in module, i tested same unit test in serialization branch, it is success!

Our issue might be related to a seemingly long-standing issue in Hibernate:
http://opensource.atlassian.com/projects/hibernate/browse/HHH-2862

I'm not exactly sure what a "non-unique collection" is though. This doesn't look like a bug in Hibernate, perhaps just an error in our mapping or schema.

Lu - Can you commit your test case to org.openmrs.module.cohort.SerializationTest.java within the reporting module.

Yep, jmiranda

I have submitted my written test method "shouldSavePersonAddress" in "org.openmrs.module.cohort.SerializationTest.java" within the reproting module.

Yes, all these days i am looking much topics about the solution for "LazyInitializationException" (also containing http://opensource.atlassian.com/projects/hibernate/browse/HHH-2862), but still haven't think about how to resolve this ticket :-/

Today, when i am thinking about how to resolve this ticket, i find an important problem, because it maybe have a very important relationship to this ticket.

I have written a "SerializationTest.java" and submitted it to trunk's package "org.openmrs.serialization.xstream". And also i added a test method "shouldReturnSameClassName" in "SerializationTest.java" within reporting module.

Ben and jmiranda can you spend a little time on running these two unit tests and help me to know why in trunk unit test run success and in reporting module unit test run fail?

Hi, jmiranda

I have added a test method "shouldSaveCohortDefinitionUsingServiceNoWithAuthenticatedUser" in class "shouldSaveCohortDefinitionUsingServiceNoWithAuthenticatedUser" and after do some extra work, that test method can run successfully.

The extra work are as follow:

(1) modify file "User.hbm.xml" of openmrs-trunk 1.5 which reporting module should base on.

the modifying content as follow:
<map name="userProperties" table="user_property" lazy="false"
cascade="save-update,merge,evict">
......
</map>

we let "User.userProperties" to be eager fetched.

Note: also should re-run "package-api" target and copy new "openmrs-api-xxx.jar" to reporting module.

(2) Replace cohortDefinition.creator from "Context.getAuthenticatedUser" to "Context.getUserService().getUser(1)"

After do above (1) and (2) steps, that new added test method can run successfully.

Note: For convenience, i attatched the modified "User.hbm.xml" here, you can directly copy its content.

After i have modify above two steps to let serialization work successfully be runned in reporting module.

I find a very strange question, it is that why i have to replace "Context.getAuthenticatedUser()" with "Context.getUserService().getUser(1)" to let serialization work run successfully in module?

Maybe, there is some wrong places in "Context.getAuthenticatedUser()" about dealing with Hibernate session?

patch for this ticket

Hi, jmiranda

I have attached a patch for this ticket, you can have a try to whether this patch can work, it work successfully in my machine.

And in my machine, i let reporting module based on openmrs trunk build 1.6.0.8890

Excellent find Lu! Are you sure you need both changes though? I would rather not make userProperties non-lazy. :-/

Hi, Ben

In fact, i think if we don't want to let userProperties as non-lazy, then we must resolve another question.

It is that if i through such a statement

PersonAddress pa = Context.getPersonService().getPersonAddressByUuid("0908e42c-ff9b-4f0c-be5c-4a517e3feb34");

For this "pa", its creator and its person are same one, both is the admin.

In reporting module "pa.getCreator()" has been initialized, but "pa.getPerson()" hasn't been initialized and it's a cglib proxy, why? They are the same one, i am not know why one was initialized, but one was not initialized?

But In trunk, both them was initialized, i think we should find the difference between module and trunk about hibernate's initialization.

How are you running the test? Are you running only one of them? Is getPerson() called in another test that initializes it?

Ah ha! Its because the reporting module is using the wrong hibernate jar. That jar has some hacks in it that didn't always play well with our objects. I replaced the hbm jar with the trunk jar and your test passes in the reporting module as well.

So with this passing, the only fix is to get rid of the clearSession()? What if we leave the clearSession and just add a call to Context.refreshAuthenticatedUser()? (quite hacky, but might work)

Cool, Ben

I have researched many days, but haven't found that it is because that trunk and reporting module are not using the same hibernate jar.

Thanks, Ben

Yep, you are right, i think we can use Context.refreshAuthenticatedUser() replacing with clearSession. And where do you want to place Context.refreshAuthenticatedUser() in?

Does it work if you put it right after the clearSession() in BaseContextSensitiveTest?

It's ok, Ben

I have put it right after the clearSession() and tested "CohortDefinitionServiceTest" within reporting module successfully, just now.

I think we can close this ticket now, do you think so?

And i think ticket TRAC-1604,TRAC-1608,TRAC-1609 were also fixed resulting from us resolving this ticket.

Added refreshAuthUser call to trunk in rev:8918. Backported to 1.5.x in rev:8919.

Justin, I will backport the serialization fixes from trunk to 1.5.x. After that, can you confirm that it solves your problems and close this and any related tickets?

Ben, thanks for your work to backport the serialization fixes from trunk to 1.5.x

The test class passes, but the module still fails (on persisting any serialized objects) when running it in the 1.5.x WAR.

I tried saving a report schema from the UI and even added a Context.refreshAuthenticatedUser() call before the saveObject() call ... to no avail.

public ReportSchema saveReportSchema(ReportSchema reportSchema) throws APIException \{	
	Context.refreshAuthenticatedUser();
	return serializedObjectDAO.saveObject(reportSchema);
\}

I must be missing something here. I actually tried the refreshAuthenticatedUser() approach awhile back but that didn't help. I imagine the "caching" of the authenticated user in the ThreadLocal is significantly different when running in a WAR vs. in a JUNIT test, right?

Maybe that is the reason it might still be failing in the webapp?

If we don't resolve this in the next 24 hours, I'm going to go back to using the revision prior to the latest 1.5.x changeset since that revision was "working" (as long as I manually removed the proxy class from the serialized XML between tomcat restarts). Now, I'm back to the scenario I was a week ago, running off the same code that was in trunk and not being able to save anything.

FYI, saving cohort definitions from the UI also continues to throw the the lazy initialization exception. I'll continue to play around and see if there's a workaround.

I was able to save the cohort definition by rewriting the CohortDefinitionPersister.saveCohortDefinition() method as follows:

public CohortDefinition saveCohortDefinition(CohortDefinition cohortDefinition) \{    	
    	User authUser = Context.getAuthenticatedUser();
    	User user = Context.getUserService().getUser(authUser.getId());   	
    	cohortDefinition.setCreator(user);
    	cohortDefinition.setChangedBy(user);
    	return dao.saveObject(cohortDefinition);
    \}

This appears to be what Context.refreshAuthenticateUser(), except changing the code to call this method still throws the "org.hibernate.LazyInitializationException (could not initialize proxy - no Session)" error:

public CohortDefinition saveCohortDefinition(CohortDefinition cohortDefinition) \{    	
    	Context.refreshAuthenticatedUser();
    	return dao.saveObject(cohortDefinition);
    \}

Oh, sorry, Justin

I haven't test serialization work for reporting module which is started from web app.

Do you mean now we can not get the fail information from any unit test, the only way to get the fail information is just running reporting module which is base on Openmrs 1.5.x from web app?

Yeah, I'm not sure if I can reproduce this any longer within the testing framework because I think it requires an authenticated user object to have been initialized and referenced in separate http requests (i.e. in separate transactions). I'm not sure if we can simulate that in the testing framework. Ben is it possible?

fix for the "stale" user issue

So I've added a patch that seems to fix the issue in my local development environment. Calling Context.refreshAuthenticatedUser() from within the InitializationFilter would have been the more appropriate thing to do, but this method did not get rid of the exception when it was called from the CohortDefinitionPersister and AuditableSaveHandler, so I went with this option instead.

(most commented ticket ever)

You might be able to simulate it in a test case by closing and opening a session.

This seems kind of hacky...do any of handlers use this? Why does this not effect other places in code? I'd rather this be done by the getAuthenticatedUser() call. Is there a way to tell if it was loaded in another session and so needs to be "refreshed" ?

(It would be more appropriate to put the call to refreshAuthUser() in the OpenmrsFilter than the InitializationFilter actually...)

I tried reproducing the error by wrapping openSession()/closeSession() around an authentication call and then again around a saveCohortDefinition() call. This did not have the intended effects. In addition, I tried to replicate the OpenmrsFilter.doFilter() method within @Before/@After methods, but that did not work as expected either (although I suspect I might have been doing something wrong there).

Anyway, I'm going to commit my change to trunk and backport to 1.5.x. If someone wants to refactor this change and send me a patch to test, by all means, but I can't work on this any longer.

FYI, the test class was moved to "reporting/test/api/src/org/openmrs/module/serialization/SerializationTest.java" to keep it separate from the other reporting tests.

a final patch that will be committed to trunk and backported to 1.5.x

Fixed

• Committed to trunk in changeset rev:9387
• Backported to branch 1.5.x in changeset rev:9388

Reopened because the patch causes problems in other areas (like TRAC-1688).

The alternative, setting lazy="false" for the User.userProperties does not seem to fix the issue as I continue to see "org.hibernate.LazyInitializationException: could not initialize proxy - no Session" when saving reporting objects. I also made the Person->User and Person->Patient associations non-lazy, but that did not help either.

Hi, Justin

Can you tell me which unit tests i can reproduce this error?

Hmm, at the beginning, when i modified the lazy="false" for the User.userProperties, the exception gone away. I don't know why this exception exist agian now.

This only happens with the LONG serializer, which I had unintentionally been using for the past week (since I switched to the module). I switched back to the SHORT serializer yesterday and the problem went away for me. I think the lazy=false works (it doesn't seem to be the user.userProperties association that fails to initialize). In fact, the latest stacktrace shows the LazyInit exception being thrown from within the User.hashCode() method. I'll post the latest stacktrace above.

latest stacktrace

The decision out of today's conference call: http://openmrs.org/wiki/2009-07-30_Developers_Conference_Call was for Justin to revert the "refreshUser" changes.

This leaves the error in the long serializer in the xstream module needing to be fixed. The ticket for that is: TRAC-1701

I beat you to the change, Justin. 1.5.x: rev:9557 and trunk in rev:9559

Thanks Ben. I was going to get to this tonight. Looks like I'm 6 hours too late.