Re: [Kernow] DirectoryTransformer.runDirectoryTransform() on "not a directory"
Brought to you by:
ajwelch
From: Andrew W. <and...@gm...> - 2007-08-06 16:31:15
|
On 8/6/07, Florent Georges <dar...@ya...> wrote: > Still about errors in runDirectoryTransform() and the > boolean it returns... Near the end of the method, you have > the following lines of code: > > } catch (InterruptedException ex) { > threadPool.shutdownNow(); > } catch (ExecutionException ex) { > ex.printStackTrace(); > } > > // Finished or cancelled, so tell the user > if (!cancelled & !stoppedOnAnError) { > // Work out the time taken > long endTime = System.currentTimeMillis(); > String time = Timer.getTimeTaken(startTime, endTime); > Message.info("\n" + xmlFiles.size() + " files processed in " + > time + "\n\n"); > progressText = "Done " + (overallSuccess?"":"(with failures) ") > + "in " + time; > overallSuccess = true; > } else if (cancelled) { > progressText = "Cancelled"; > threadPool.shutdownNow(); > } else if (stoppedOnAnError) { > progressText = "Stopped on an error"; > } > > } catch (TransformerConfigurationException ex) { > progressText = "Error in stylesheet"; > Message.exception(ex, true); > } > > I wonder if some failure should be added in those lines > (that is, setting overallSuccess to false). Please see the > comments in the code (tagged MAYBE, ADDED and REMOVED): > > } catch (InterruptedException ex) { > // MAYBE? I'm not sure if we should add "overallSuccess = > // false" here, as well as a message in "progressText" ? > // I think not here, but yes just below. > threadPool.shutdownNow(); No need here I think, as should only ever get here after clicking cancel. > } catch (ExecutionException ex) { > // MAYBE? Same as above > ex.printStackTrace(); > } Yes, probably should set it here. > // Finished or cancelled, so tell the user > if (!cancelled & !stoppedOnAnError) { > // Work out the time taken > long endTime = System.currentTimeMillis(); > String time = Timer.getTimeTaken(startTime, endTime); > Message.info("\n" + xmlFiles.size() + " files processed in " + > time + "\n\n"); > progressText = "Done " + (overallSuccess?"":"(with failures) ") > + "in " + time; > // REMOVED "overallSuccess = true" > // Because if it was set to false earlier, there is > // no reason to set it to true here Yes it looks like you're correct, here too. > } else if (cancelled) { > progressText = "Cancelled"; > overallSuccess = false; // ADDED > threadPool.shutdownNow(); > } else if (stoppedOnAnError) { > progressText = "Stopped on an error"; > overallSuccess = false; // ADDED > } Cool, thanks. > } catch (TransformerConfigurationException ex) { > progressText = "Error in stylesheet"; > overallSuccess = false; // ADDED > Message.exception(ex, true); > } > > What do you think? I had to double check as initially I thought my logic was the wrong way around and overallSuccess should be set to false and only ever set to true when it gets to the end of the try block (like in all the other classes)... however because of the ability to continue the directory transform even if some of the transforms fail it has to be done this way, hence the need to set it to false in several places (several that I seem to have missed :) > By the way, I don't know a simple way to test such thread > issues (for example to test that if a thread is interrupted > the method should fail, etcetera). Do you know? No, sorry (but then I don't think its too bad if that part is tested) -- http://andrewjwelch.com |