Changeset 31692 for main/trunk/gli/src/org/greenstone/gatherer/download/DownloadProgressBar.java

Timestamp:

2017-05-18T20:38:56+12:00 (7 years ago)

Author:

ak19

Message:

Major changes to SafeProcess and classes of the download package, by bringing the final GLI (and final Greenstone) class DownloadJob over to use SafeProcess. Significant changes to SafeProcess: 1. Introduction of cancelRunningProcess as a new method, so that callers don't need to know how cancelling a process is implemented (as an interrupt) nor do they need to know what thread they ought to interrupt (which should be the thread that launched SafeProcess), nor do they need to know of the complexity surrounding the join() blocking call which should not be interrupted. 2. Introduction of the SafeProcess.MainHandler interface that provides methods that allow implementers to write hooks to various stages of the SafeProcess' internal process' life cycle. 3. moved process cleanUp() code into a reusable method within SafeProcess. Significant changes to DownloadJob and its associated DownloadProgressBar and DownloadScrollPane classes: 1. Unused member vars removed or commented out and those that can be declared final are now declared so, as a programming pricinple for thread safety, since DownloadJob and the other download classes will have to interact with multiple threads and could be called by different threads. 2. Replaced existing actionPerformed() and callDownload() of DownloadJob with SafeProcess and implemented the necessary Hooks in the SafeProcess.MainHandler() interface to ensure that perl is still told to terminate wget on a cancel action. 3. Replaced DownloadScrollPane's deleteDownloadJob() with a new version that now more responsively removes its DownloadProgressBar (with controls) for a DownloadJob. It's called by the SafeProcess.MainHandler interface hooks implemented by DownloadJob, so DownloadScrollPane no longer needs to wait() to be notify()ed when a process has cleaned up on premature termination by a cancel action. 4. Made the necessary methods in DownloadProgressBar synchronized for thread safety. 5. GShell now uses SafeProcess' new cancelRunningProcess() method in place of directly calling interrupt on the (GShell) thread that launched SafeProcess.

File:

• main/trunk/gli/src/org/greenstone/gatherer/download/DownloadProgressBar.java (modified) (11 diffs)

main/trunk/gli/src/org/greenstone/gatherer/download/DownloadProgressBar.java

@@ -128 +128 @@
     JPanel button_pane = new JPanel();
 
+    // our "pause" button never paused before, it always did a process.destroy() on being pressed.
+    // See http://trac.greenstone.org/browser/trunk/gli/src/org/greenstone/gatherer/download/DownloadJob.java?rev=13594
+    // However, now we additionally ensure that the wget launched by the perl is stopped before 
+    // process.destroy(), so at least it cleans up better. I'm therefore changing it to a "Stop" button.
     stop_start_button = new GLIButton(Dictionary.get("Mirroring.DownloadJob.Pause"),Dictionary.get("Mirroring.DownloadJob.Pause_Tooltip"));
+    //stop_start_button = new GLIButton(Dictionary.get("Mirroring.DownloadJob.Stop"),Dictionary.get("Mirroring.DownloadJob.Stop_Tooltip"));
     stop_start_button.addActionListener(this);
     stop_start_button.addActionListener(owner);
@@ -166 +171 @@
     // Make the labels, etc update.
     refresh();
+    }
+
+    // internally thread-safe
+    public void enableCancelJob(boolean isEnabled) {
+    synchronized(stop_start_button) {
+        stop_start_button.setEnabled(isEnabled);
+    }
+    synchronized(stop_start_button) {
+        close_button.setEnabled(isEnabled);
+    }
     }
 
@@ -212 +227 @@
      * @param url The url String of the file that is being downloaded.
      */
-    public void addDownload(String url) {
+    public synchronized void addDownload(String url) {
     current_url = url;
     file_size = 0;
@@ -221 +236 @@
      * is called to enlighten the DownloadProgressBar of this fact.
      */
-    public void downloadComplete() {
+    public synchronized void downloadComplete() {
     current_url = null;
     file_count++;
@@ -232 +247 @@
     }
 
-    public void downloadFailed() {
+    public synchronized void downloadFailed() {
     err_count++;
     if(total_count < (file_count + err_count + warning_count)) {
@@ -240 +255 @@
     }
 
-    public void downloadWarning() {
+    public synchronized void downloadWarning() {
     warning_count++;
     if(total_count < (file_count + err_count + warning_count)) {
@@ -248 +263 @@
     }
 
-    public void setTotalDownload(int total_download) {
+    // need to make these methods synchronized too, as they modify variables
+    // that other synchronized methods work with. And if any methods that modify
+    // such variables were to remain unsynchronized, can end up with race conditions
+    // http://stackoverflow.com/questions/574240/is-there-an-advantage-to-use-a-synchronized-method-instead-of-a-synchronized-blo
+    // "Not only do synchronized methods not lock the whole class, but they don't lock the whole instance either. Unsynchronized methods in the class may still proceed on the instance."
+    // "Only the syncronized methods are locked. If there are fields you use within synced methods that are accessed by unsynced methods, you can run into race conditions."
+    public synchronized void setTotalDownload(int total_download) {
     total_count = total_download;
     refresh();
@@ -257 +278 @@
     }
 
-    public void increaseFileCount() {
+    public synchronized void increaseFileCount() {
     file_count++;
     refresh();
     }
 
-    public void increaseFileCount(int amount) {
+    public synchronized void increaseFileCount(int amount) {
     file_count += amount;
     refresh();
     }
 
-    public void resetFileCount() {
+    public synchronized void resetFileCount() {
     file_count = 0;
     refresh();
@@ -278 +299 @@
      * reset to zero.
      */
-    public void mirrorBegun(boolean reset, boolean simple) {
+    public synchronized void mirrorBegun(boolean reset, boolean simple) {
     if(reset) {
         this.file_count = 0;
@@ -301 +322 @@
      * components.
      */
-    public void mirrorComplete() {
+    public synchronized void mirrorComplete() {
     current_action = DownloadJob.COMPLETE;
     current_url = null;
@@ -319 +340 @@
      * the amount of the current file downloaded.
      */
-    public void updateProgress(long current, long expected) {
+    public synchronized void updateProgress(long current, long expected) {
     file_size = file_size + current;
     if(!progress.isIndeterminate()) {