imglib / imglib2-algorithm-gpl

Image processing algorithms for ImgLib2 (GPL-licensed)
http://imglib2.net/
GNU General Public License v2.0
3 stars 6 forks source link

Fix ExecutorService not correctly shutdown #3

Closed hadim closed 9 years ago

hadim commented 9 years ago

Fix ExecutorService not correctly shutdown at the end of FFTConvolution.convolve().

See https://github.com/imglib/imglib2-algorithm-gpl/blob/master/src/main/java/net/imglib2/algorithm/fft2/FFTConvolution.java#L547

and the related bug in Trackmate : https://github.com/fiji/TrackMate/issues/59

hadim commented 9 years ago

I confirm this PR fix the Trackmate issue (fiji/TrackMate#59). Tested on two computers with two different scripts.

ctrueden commented 9 years ago

Great catch, @hadim!

hadim commented 9 years ago

Well that wasn't so hard after all :-)

I also confirm the fix on a Windows machine using Micro-Manager.

Good to merge in my opinion.

ctrueden commented 9 years ago

Agreed, LGTM.

StephanPreibisch commented 9 years ago

Thanks so much @hadim!

tinevez commented 9 years ago

@tpietzsch Could we release imglib2-algorithm-gpl so as to release the fix?

tpietzsch commented 9 years ago

@tinevez done.

tinevez commented 9 years ago

Thanks!

hadim commented 9 years ago

What about this bug https://github.com/fiji/TrackMate/issues/59 then ?

tpietzsch commented 9 years ago

@hadim @tinevez As @cdietz suspected, TrackMate provides an ExecutorService from outside, which it doesn't close. This is here: https://github.com/fiji/TrackMate/blob/master/src/main/java/fiji/plugin/trackmate/detection/LogDetector.java#L133-L134 It should be something like this:

        ExecutorService service = Executors.newFixedThreadPool( numThreads );
        fftconv.setExecutorService(service);
        fftconv.convolve();
        service.shutdown();
hadim commented 9 years ago

Ok I'll check that. Thank you @tpietzsch and @dietzc. How do you want I proceed to roll back ? Should I just open a new PR ?

dietzc commented 9 years ago

yes, probably thats the easiest is to file another PR. @tpietzsch, any preferences?

tpietzsch commented 9 years ago

@hadim new PR is fine. Thanks for working on this!

hadim commented 9 years ago

@tinevez I'll also check the fix proposed by @tpietzsch and open a new PR for trackmate.

hadim commented 9 years ago

See #4.