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

Memory leak caused by ThreadGroup #10394

Closed
sancar opened this issue Apr 20, 2017 · 0 comments
Closed

Memory leak caused by ThreadGroup #10394

sancar opened this issue Apr 20, 2017 · 0 comments

Comments

@sancar
Copy link
Member

@sancar sancar commented Apr 20, 2017

Issue reported in google group. Following content is copied from https://groups.google.com/d/msg/hazelcast/luiFu_WN_Io/eGFKM6HmAwAJ


I've been doing reliability testing and found a leak with 3.8, also present with 3.8.1. The specific case I was testing was what happens in a Hazelcast client if the server is not available for a long time and the client keeps trying to connect. I had been running it for days with connections every few seconds and eventually it crashed with the heap full. I investigated the reason and found it to be as below.

In short, for every connection, there are 3 instances of java.lang.ThreadGroup that stay around. They belong to Hazelcast (their name is hz_client_NUMBER).

Further testing showed the leak occurs with successful connections as well.

A project to reproduce the problem in its simplest form, with instructions and screenshots from the profiler, is available at https://github.com/jl2008/hazelcast-client-test-threadgroup

The client is shut down in all cases, so I didn't expect a leak:

HazelcastInstance client = null;
try {
   client = HazelcastClient.newHazelcastClient(hazelcastClientConfig); //has to be inside try, the constructor throws an exception if the cluster is unavailable
} catch (Exception e) {
   //...
} finally {
   if (client != null) {
      client.shutdown();
   }
}

Let me know if I've done something wrong or I can assist with anything else,
JL

@sancar sancar added this to the 3.8.2 milestone Apr 20, 2017
@sancar sancar self-assigned this Apr 20, 2017
sancar added a commit to sancar/hazelcast that referenced this issue Apr 20, 2017
Since thread group is adding itself to a list in parent thread
group, it is not garbage collected. One has to call threadGroup.destroy
to remove it back from the list.

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 20, 2017
Since thread group is adding itself to a list in parent thread
group, it is not garbage collected. One has to call threadGroup.destroy
to remove it back from the list.

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 20, 2017
Since thread group is adding itself to a list in parent thread
group, it is not garbage collected. One has to call threadGroup.destroy
to remove it back from the list.

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 20, 2017
Since thread group is adding itself to a list in parent thread
group, it is not garbage collected. One has to call threadGroup.destroy
to remove it back from the list.

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 20, 2017
Since thread group is adding itself to a list in parent thread
group, it is not garbage collected. One has to call threadGroup.destroy
to remove it back from the list.

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 20, 2017
Since thread group is adding itself to a list in parent thread
group, it is not garbage collected. One has to call threadGroup.destroy
to remove it back from the list.

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 24, 2017
Since thread group is adding itself to a list in parent thread
group, it is not garbage collected. One has to call threadGroup.destroy
to remove it back from the list.

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 24, 2017
Since thread group is adding itself to a list in parent thread
group, it is not garbage collected. One has to call threadGroup.destroy
to remove it back from the list.

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 27, 2017
Since ThreadGroup is adding itself to its parent list in constructor,
one has to call ThreadGroup.destroy to remove it back from the list.
ThreadGroup.destroy has to be called after all threads are finished.
We were not waiting for threads to finish before, to avoid memory leak
it seems we have to. But there is a caveat.

It seems impossible to wait for all threads to finish succesfully
because the shutdown thread is also waiting itself to finish.
There is no way to create a Thread adding itself to ThreadGroup
of parent. Hence shutdowns initiated in one of our own threads
has to wait itself to finish.

As a solution, this pr removed the ThreadGroup usage completely.
Justification 1:
We are using ThreadGroups to be able to interrupt all running threads.
As long as we don't create free threads(without executor service), this
interrupting is not necessary. And when we create a thread without executor
service, it is on purpose such as shutdown thread.

Justification 2: A note from Effective Java 2nd Ed., Item 73
"The API that lists the subgroups of a thread group is similarly flawed. While these problems could have been fixed with the addition of new methods, they haven’t, because there is no real need: thread groups are obsolete.
Prior to release 1.5, there was one small piece of functionality that was available only with the ThreadGroup API: the ThreadGroup.uncaughtException method was the only way to gain control when a thread threw an uncaught exception. This functionality is useful, for example, to direct stack traces to an application- specific log. As of release 1.5, however, the same functionality is available with Thread’s setUncaughtExceptionHandler method.
To summarize, thread groups don’t provide much in the way of useful functionality, and much of the functionality they do provide is flawed. Thread groups are best viewed as an unsuccessful experiment, and you should simply ignore their existence. If you design a class that deals with logical groups of threads, you should probably use thread pool executors (Item 68)."

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 27, 2017
Since ThreadGroup is adding itself to its parent list in constructor,
one has to call ThreadGroup.destroy to remove it back from the list.
ThreadGroup.destroy has to be called after all threads are finished.
We were not waiting for threads to finish before, to avoid memory leak
it seems we have to. But there is a caveat.

It seems impossible to wait for all threads to finish succesfully
because the shutdown thread is also waiting itself to finish.
There is no way to create a Thread adding itself to ThreadGroup
of parent. Hence shutdowns initiated in one of our own threads
has to wait itself to finish.

As a solution, this pr removed the ThreadGroup usage completely.
Justification 1:
We are using ThreadGroups to be able to interrupt all running threads.
As long as we don't create free threads(without executor service), this
interrupting is not necessary. And when we create a thread without executor
service, it is on purpose such as shutdown thread.

Justification 2: A note from Effective Java 2nd Ed., Item 73
"The API that lists the subgroups of a thread group is similarly flawed. While these problems could have been fixed with the addition of new methods, they haven’t, because there is no real need: thread groups are obsolete.
Prior to release 1.5, there was one small piece of functionality that was available only with the ThreadGroup API: the ThreadGroup.uncaughtException method was the only way to gain control when a thread threw an uncaught exception. This functionality is useful, for example, to direct stack traces to an application- specific log. As of release 1.5, however, the same functionality is available with Thread’s setUncaughtExceptionHandler method.
To summarize, thread groups don’t provide much in the way of useful functionality, and much of the functionality they do provide is flawed. Thread groups are best viewed as an unsuccessful experiment, and you should simply ignore their existence. If you design a class that deals with logical groups of threads, you should probably use thread pool executors (Item 68)."

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 27, 2017
Since ThreadGroup is adding itself to its parent list in constructor,
one has to call ThreadGroup.destroy to remove it back from the list.
ThreadGroup.destroy has to be called after all threads are finished.
We were not waiting for threads to finish before, to avoid memory leak
it seems we have to. But there is a caveat.

It seems impossible to wait for all threads to finish succesfully
because the shutdown thread is also waiting itself to finish.
There is no way to create a Thread adding itself to ThreadGroup
of parent. Hence shutdowns initiated in one of our own threads
has to wait itself to finish.

As a solution, this pr removed the ThreadGroup usage completely.
Justification 1:
We are using ThreadGroups to be able to interrupt all running threads.
As long as we don't create free threads(without executor service), this
interrupting is not necessary. And when we create a thread without executor
service, it is on purpose such as shutdown thread.

Justification 2: A note from Effective Java 2nd Ed., Item 73
"The API that lists the subgroups of a thread group is similarly flawed. While these problems could have been fixed with the addition of new methods, they haven’t, because there is no real need: thread groups are obsolete.
Prior to release 1.5, there was one small piece of functionality that was available only with the ThreadGroup API: the ThreadGroup.uncaughtException method was the only way to gain control when a thread threw an uncaught exception. This functionality is useful, for example, to direct stack traces to an application- specific log. As of release 1.5, however, the same functionality is available with Thread’s setUncaughtExceptionHandler method.
To summarize, thread groups don’t provide much in the way of useful functionality, and much of the functionality they do provide is flawed. Thread groups are best viewed as an unsuccessful experiment, and you should simply ignore their existence. If you design a class that deals with logical groups of threads, you should probably use thread pool executors (Item 68)."

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 27, 2017
Since ThreadGroup is adding itself to its parent list in constructor,
one has to call ThreadGroup.destroy to remove it back from the list.
ThreadGroup.destroy has to be called after all threads are finished.
We were not waiting for threads to finish before, to avoid memory leak
it seems we have to. But there is a caveat.

It seems impossible to wait for all threads to finish succesfully
because the shutdown thread is also waiting itself to finish.
There is no way to create a Thread adding itself to ThreadGroup
of parent. Hence shutdowns initiated in one of our own threads
has to wait itself to finish.

As a solution, this pr removed the ThreadGroup usage completely.
Justification 1:
We are using ThreadGroups to be able to interrupt all running threads.
As long as we don't create free threads(without executor service), this
interrupting is not necessary. And when we create a thread without executor
service, it is on purpose such as shutdown thread.

Justification 2: A note from Effective Java 2nd Ed., Item 73
"The API that lists the subgroups of a thread group is similarly flawed. While these problems could have been fixed with the addition of new methods, they haven’t, because there is no real need: thread groups are obsolete.
Prior to release 1.5, there was one small piece of functionality that was available only with the ThreadGroup API: the ThreadGroup.uncaughtException method was the only way to gain control when a thread threw an uncaught exception. This functionality is useful, for example, to direct stack traces to an application- specific log. As of release 1.5, however, the same functionality is available with Thread’s setUncaughtExceptionHandler method.
To summarize, thread groups don’t provide much in the way of useful functionality, and much of the functionality they do provide is flawed. Thread groups are best viewed as an unsuccessful experiment, and you should simply ignore their existence. If you design a class that deals with logical groups of threads, you should probably use thread pool executors (Item 68)."

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 27, 2017
Since ThreadGroup is adding itself to its parent list in constructor,
one has to call ThreadGroup.destroy to remove it back from the list.
ThreadGroup.destroy has to be called after all threads are finished.
We were not waiting for threads to finish before, to avoid memory leak
it seems we have to. But there is a caveat.

It seems impossible to wait for all threads to finish succesfully
because the shutdown thread is also waiting itself to finish.
There is no way to create a Thread adding itself to ThreadGroup
of parent. Hence shutdowns initiated in one of our own threads
has to wait itself to finish.

As a solution, this pr removed the ThreadGroup usage completely.
Justification 1:
We are using ThreadGroups to be able to interrupt all running threads.
As long as we don't create free threads(without executor service), this
interrupting is not necessary. And when we create a thread without executor
service, it is on purpose such as shutdown thread.

Justification 2: A note from Effective Java 2nd Ed., Item 73
"The API that lists the subgroups of a thread group is similarly flawed. While these problems could have been fixed with the addition of new methods, they haven’t, because there is no real need: thread groups are obsolete.
Prior to release 1.5, there was one small piece of functionality that was available only with the ThreadGroup API: the ThreadGroup.uncaughtException method was the only way to gain control when a thread threw an uncaught exception. This functionality is useful, for example, to direct stack traces to an application- specific log. As of release 1.5, however, the same functionality is available with Thread’s setUncaughtExceptionHandler method.
To summarize, thread groups don’t provide much in the way of useful functionality, and much of the functionality they do provide is flawed. Thread groups are best viewed as an unsuccessful experiment, and you should simply ignore their existence. If you design a class that deals with logical groups of threads, you should probably use thread pool executors (Item 68)."

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 27, 2017
Since ThreadGroup is adding itself to its parent list in constructor,
one has to call ThreadGroup.destroy to remove it back from the list.
ThreadGroup.destroy has to be called after all threads are finished.
We were not waiting for threads to finish before, to avoid memory leak
it seems we have to. But there is a caveat.

It seems impossible to wait for all threads to finish succesfully
because the shutdown thread is also waiting itself to finish.
There is no way to create a Thread adding itself to ThreadGroup
of parent. Hence shutdowns initiated in one of our own threads
has to wait itself to finish.

As a solution, this pr removed the ThreadGroup usage completely.
Justification 1:
We are using ThreadGroups to be able to interrupt all running threads.
As long as we don't create free threads(without executor service), this
interrupting is not necessary. And when we create a thread without executor
service, it is on purpose such as shutdown thread.

Justification 2: A note from Effective Java 2nd Ed., Item 73
"The API that lists the subgroups of a thread group is similarly flawed. While these problems could have been fixed with the addition of new methods, they haven’t, because there is no real need: thread groups are obsolete.
Prior to release 1.5, there was one small piece of functionality that was available only with the ThreadGroup API: the ThreadGroup.uncaughtException method was the only way to gain control when a thread threw an uncaught exception. This functionality is useful, for example, to direct stack traces to an application- specific log. As of release 1.5, however, the same functionality is available with Thread’s setUncaughtExceptionHandler method.
To summarize, thread groups don’t provide much in the way of useful functionality, and much of the functionality they do provide is flawed. Thread groups are best viewed as an unsuccessful experiment, and you should simply ignore their existence. If you design a class that deals with logical groups of threads, you should probably use thread pool executors (Item 68)."

fixes hazelcast#10394
sancar added a commit to sancar/hazelcast that referenced this issue Apr 28, 2017
Since ThreadGroup is adding itself to its parent list in constructor,
one has to call ThreadGroup.destroy to remove it back from the list.
ThreadGroup.destroy has to be called after all threads are finished.
We were not waiting for threads to finish before, to avoid memory leak
it seems we have to. But there is a caveat.

It seems impossible to wait for all threads to finish succesfully
because the shutdown thread is also waiting itself to finish.
There is no way to create a Thread adding itself to ThreadGroup
of parent. Hence shutdowns initiated in one of our own threads
has to wait itself to finish.

As a solution, this pr removed the ThreadGroup usage completely.
Justification 1:
We are using ThreadGroups to be able to interrupt all running threads.
As long as we don't create free threads(without executor service), this
interrupting is not necessary. And when we create a thread without executor
service, it is on purpose such as shutdown thread.

Justification 2: A note from Effective Java 2nd Ed., Item 73
"The API that lists the subgroups of a thread group is similarly flawed. While these problems could have been fixed with the addition of new methods, they haven’t, because there is no real need: thread groups are obsolete.
Prior to release 1.5, there was one small piece of functionality that was available only with the ThreadGroup API: the ThreadGroup.uncaughtException method was the only way to gain control when a thread threw an uncaught exception. This functionality is useful, for example, to direct stack traces to an application- specific log. As of release 1.5, however, the same functionality is available with Thread’s setUncaughtExceptionHandler method.
To summarize, thread groups don’t provide much in the way of useful functionality, and much of the functionality they do provide is flawed. Thread groups are best viewed as an unsuccessful experiment, and you should simply ignore their existence. If you design a class that deals with logical groups of threads, you should probably use thread pool executors (Item 68)."

fixes hazelcast#10394
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants