Re: [Bug# 228886] Multiple Glassfish threads hanging in memory

Hi,
I'm sorry, but your formulation looks like I'm ok with the patch.

What I don't like about the patch? It is a bit large and does fixes in 
area which is hard and error prone on its own (threading). It also fixes 
a lot more than just thread leak, while there is simpler solution to the 
leak itself. We already found one issue in the original patch and 
looking at the diff there is another one in the library - not 
synchronized access to eventListeners in notifyListeners. So the whole 
thing is a little bit risky from my point of view.

I have no objections about the integration if other reviewers are happy 
about the patch.
P.

On 05/21/2013 01:42 PM, Tomas Kraus wrote:
> *Bug 228886* <https://netbeans.org/bugzilla/show_bug.cgi?id=228886>
> -Multiple Glassfish threads hanging in memory
>
> Zombie thread is left after GlassFish Log Reader thread has been
> interrupted in build task.
>
> - Start NetBeans
> - Start VisualVM and open NetBeans, threads table, sort by Thread
> - Create sample project ServletStateless
> - Start GlassFish server in debug mode
> - Undeploy all projects
> Repeat the following cycle
> - Run project ServletStateless
>    creates one thread FetchLogPiped
> - Clean and build project
>    creates one thread FetchLogPiped.
>
> So if you run this cyle 3 times you should have 6 FetchLogPiped threads.
>
> Fix in GF plugin was made in 4 steps (253015:70350e3b09dc,
> 253069:422df5694675, 253094:1ad1e811d955 and 253176:e0be6fab29ed),
> combined diff is attached as
> https://netbeans.org/bugzilla/attachment.cgi?id=134564&action=diff
>
> I made single thread pool (ExecutorService) to be shared for all
> GlassFish Log Reader threads. Threads are run under top level
> ThreadGroup to not be interrupted by build task. Threads have
> setDaemon(true) set to avoid them being NB exit blockers.
>
> GlassFish Tooling Library changeset is 502:280234be9cb0.
>
> It allows to pass external ExecutorService into GlassFish Log Reader
> thread starting code.
>
> Code went trough 2 reviews - Petr Hejl and Peter Benedikovic. It was
> cleaned up thanks to Petr Hejl notes and now I believe it's safe enough
> to be pushed in to 7.3.1 release branch. QA testing was done by Jiri
> Skrivanek. More details are in the bug.
>