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

Avoid ClassLoader leaks caused by SharedTimer #664

Closed

Conversation

@mjiderhamn
Copy link
Contributor

@mjiderhamn mjiderhamn commented Oct 12, 2016

Back in PR #197 an attempt was made to avoid ClassLoader leaks by introducing SharedTimer. However, since the TimerThread started by SharedTimer inherits the contextClassLoader of the currently executing thread this could still cause leaks in an environment where a connection pool is shared between multiple ClassLoaders such as multiple web applications or multiple instances (redeploys) of the same web application, so that Connections are kept open longer than the lifespan of the ClassLoader.

Likewise the inherited AccessControlContext of the TimeThread will likely contain ProtectionDomains with references to calling classes and thus their ClassLoader, also leading to potential leaks under the same circumstances.

@davecramer
Copy link
Member

@davecramer davecramer commented Oct 13, 2016

Hi Mattias,

Thanks for the PR. Can you fix the checkstyle issues please ?

Dave Cramer

On 12 October 2016 at 16:01, Mattias Jiderhamn notifications@github.com
wrote:

I would like to investigate if we'd also need to avoid references from the
inherited AccessControlContext, but not sure when I'd find the time.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#664 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAYz9lThMnZCnaGSAxdR8I2FzB7VgUBuks5qzTyqgaJpZM4KVKH3
.

Avoid ClassLoader leaks caused by the TimerThread started by SharedTimer inheriting the contextClassLoader of the currently executing thread and the AccessControlContext containing ProtectionDomains with calling code.
@mjiderhamn mjiderhamn force-pushed the mjiderhamn:SharedTimer_contextClassLoader branch from bce3a06 to 12cfa7d Oct 13, 2016
@mjiderhamn
Copy link
Contributor Author

@mjiderhamn mjiderhamn commented Oct 13, 2016

Sorry, forgot to run mvn checkstyle:check after last tweak. Should be fixed now.

@vlsi vlsi closed this in 72562cc Oct 29, 2016
vlsi added a commit that referenced this pull request Oct 29, 2016
Avoid ClassLoader leaks caused by the TimerThread started by SharedTimer inheriting the contextClassLoader of the currently executing thread and the AccessControlContext containing ProtectionDomains with calling code.

closes #664
vlsi added a commit that referenced this pull request Oct 29, 2016
Avoid ClassLoader leaks caused by the TimerThread started by SharedTimer inheriting the contextClassLoader of the currently executing thread and the AccessControlContext containing ProtectionDomains with calling code.

closes #664
vlsi added a commit that referenced this pull request Oct 29, 2016
Avoid ClassLoader leaks caused by the TimerThread started by SharedTimer inheriting the contextClassLoader of the currently executing thread and the AccessControlContext containing ProtectionDomains with calling code.

closes #664
@vlsi
Copy link
Member

@vlsi vlsi commented Oct 29, 2016

@praiskup ,

Apparently

            <dependency>
                <groupId>se.jiderhamn</groupId>
                <artifactId>classloader-leak-test-framework</artifactId>
                <version>1.1.1</version>
                <scope>test</scope>
            </dependency>

is not yet fedora-packaged.

Do you think it can be packaged?

@praiskup
Copy link
Member

@praiskup praiskup commented Oct 31, 2016

On Saturday, October 29, 2016 1:23:18 PM CET Vladimir Sitnikov wrote:

@praiskup ,

Apparently

            <dependency>
                <groupId>se.jiderhamn</groupId>
                <artifactId>classloader-leak-test-framework</artifactId>
                <version>1.1.1</version>
                <scope>test</scope>
            </dependency>

is not yet fedora-packaged.

Ah, thanks for the info, I filled this new bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1390063

Do you think it can be packaged?

Yeah, that's probably Apache 2.0 - so unless there are platform issues or other
transitive and problematic dependencies, we should be able to package. I assume
we already (unconditionally) depend on it, right? So better to have this
packaged ASAP?

@vlsi
Copy link
Member

@vlsi vlsi commented Oct 31, 2016

I assume we already (unconditionally) depend on it, right?

Yes, that is an unconditional dependency. That is test-only dependency and making it conditional would require some xml massage.

@praiskup
Copy link
Member

@praiskup praiskup commented Nov 3, 2016

@trepik offered a help with packaging of this dependency into Fedora, we should be able to process this next week, sorry for the delays.

@praiskup
Copy link
Member

@praiskup praiskup commented Nov 3, 2016

With the Fedora CI, please continue on #637

@praiskup praiskup mentioned this pull request Nov 3, 2016
@praiskup
Copy link
Member

@praiskup praiskup commented Nov 3, 2016

OTOH, would that make sense to actually clone the parent poms project and generate the tarball from there?

@vlsi
Copy link
Member

@vlsi vlsi commented Nov 3, 2016

would that make sense to actually clone the parent poms project and generate the tarball from there?

Can you please rephrase? I don't get the question.

@praiskup
Copy link
Member

@praiskup praiskup commented Nov 3, 2016

For the testing builds in Copr -> would there make sense to actually generate parent-poms tarball from git, instead of downloading the released one?

@vlsi
Copy link
Member

@vlsi vlsi commented Nov 3, 2016

pgjdbc always references to a released version of parent-poms

@praiskup
Copy link
Member

@praiskup praiskup commented Nov 3, 2016

So the answer is "no", because the parent poms git HEAD might be incompatible with pgjdbc git HEAD, right?

@vlsi
Copy link
Member

@vlsi vlsi commented Nov 3, 2016

right

@vlsi
Copy link
Member

@vlsi vlsi commented Nov 3, 2016

@praiskup , pgjdbc's travis build is green again

@praiskup
Copy link
Member

@praiskup praiskup commented Nov 3, 2016

@vlsi, I have some changes I believe are worth doing, give me few minutes please

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.