Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BEANUTILS-566: Use ContextClassLoader for Instantiating Loggers #267

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

volosied
Copy link

@volosied volosied commented Jul 30, 2024

https://issues.apache.org/jira/browse/BEANUTILS-566

Swaps our the thread context classloader to avoid memory leaks in some scenarios.

@garydgregory
Copy link
Member

garydgregory commented Jul 30, 2024

20 files changed files and zero tests is not good.

Try and see if Mockito helps.

@volosied volosied marked this pull request as draft July 30, 2024 14:14
@volosied volosied changed the title Fix BeanUtils-556 BeanUtils-556 : Use ContextClassLoader for Instantiating Loggers Jul 31, 2024
@volosied volosied changed the title BeanUtils-556 : Use ContextClassLoader for Instantiating Loggers BEANUTILS-566: Use ContextClassLoader for Instantiating Loggers Jul 31, 2024
@volosied
Copy link
Author

volosied commented Jul 31, 2024

Hi @garydgregory ,

I've found a way to test that the context classloader is used. I also tested the negative case to verify it's not called. There may be an easier way to test, but I simply created my own classloader and load the tests from the target/test-classes directory. I tried to add comments to make it more clear on what is occurring.

I'm not sure how to run the jobs -- they failed due to check-style violations which I've since fixed.

I would appreciate any feedback for improvements. Thank you.

@volosied
Copy link
Author

I pushed up another test which also let the builds run.

Everything passed except for Java 23, but those failures are unrelated to my changes from the looks of.

 Error:  Failures: 
Error:    SqlTimeConverterTestCase.testLocale:118->AbstractDateConverterTest.validConversion:572 Converting 'java.lang.String' value '3:06 pm' threw org.apache.commons.beanutils2.ConversionException: Error converting 'String' to 'java.sql.Time' using pattern 'h:mm a', localized pattern 'h:mm a', errorIndex 4, calendar type GregorianCalendar, this org.apache.commons.beanutils2.sql.converters.SqlTimeConverter[UseDefault=false, UseLocaleFormat=true]
Error:    SqlTimestampConverterTestCase.testLocale:138->AbstractDateConverterTest.validConversion:572 Converting 'java.lang.String' value '3/21/06, 3:06 PM' threw org.apache.commons.beanutils2.ConversionException: Error converting 'String' to 'java.sql.Timestamp' using pattern 'M/d/yy, h:mm a', localized pattern 'M/d/yy, h:mm a', errorIndex 13, calendar type GregorianCalendar, this org.apache.commons.beanutils2.sql.converters.SqlTimestampConverter[UseDefault=false, UseLocaleFormat=true]
[INFO] 

@volosied volosied marked this pull request as ready for review August 1, 2024 13:58
@volosied
Copy link
Author

volosied commented Aug 27, 2024

@garydgregory following up again for any feedback. And if the community would prefer not to use this change, that's fine, but, please let me know. Thank you.

Copy link

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@volosied,

IMHO the TCCL problem should be solved at the source in Commons Logging.
See LOGGING-196 for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants