Uploading an input file to a web server

Question

I have a long-running task that processes an input file and then uploads it to a web server. The processing part can be interrupted (with cleanup) but the upload shouldn't be interruptable so that we know whether the server received the file correctly.

My thread looks like this (I'm using PyQt but I imagine this problem occurs in other threading modules too):

class ImportThread(QtCore.QThread):
    def __init__(self, path):
        super(ImportThread, self).__init__()

        self.path = path
        self.stopped = False

    @property
    def stopped(self):
        return self._stopped

    @stopped.setter
    def stopped(self, stopped):
        self._stopped = stopped

    def run(self):
        self.emit(QtCore.SIGNAL("processing()"))

        # Do the processing task unless the thread has already been stopped
        if not self.stopped:
            do_processing_task()

            # Don't move on to the next step if the thread has been stopped
            if not self.stopped:
                self.emit(QtCore.SIGNAL("uploading()"))
            else:
                cleanup_processed_file()
                return   # Exit thread

        do_upload_task()
        self.emit(QtCore.SIGNAL("completed()"))

I don't think the nested ifs are very readable and am looking for a way to clean this up. Any suggestions?

Usage looks a little like this:

# triggering thread
self.thread = ImportThread(path)
# ... connects() to thread signals that update the progress dialog ...
self.thread.start()

# Create a progress dialog to monitor the thread
self.progress = QtGui.QProgressDialog("Starting...", "Cancel", 0, 2, self)
self.progress.setValue(0)
self.progress.show()

# Connect cancellation signal from progress dialog to method to stop the thread
self.connect(self.progress, QtCore.SIGNAL("canceled()"), self.notify_cancelled)

def notify_cancelled(self):
    self.progress.setLabelText("Cancelling...")
    self.thread.stopped = True
    self.thread.exit()    # Not sure this is correct?
    self.progress.close()

Answer 1

The code itself is readable: it makes sense to check if the thread is still running before doing extra work. It's not the usual if spaghetti, and is, I think, a common threading idiom. But it's hard to understand the logic behind the code and I'm not sure it works as intended.

The first issue is that you're emitting the uploading signal only when processing has been done, but it's theoretically possible that upload happens even without processing, if the first if not self.stopped is false. If you don't want this to happen, I suggest that you move the do_upload_task() call in the second if not self.stopped.

The second issue is that it's very unlikely that self.stopped would be false just after run() has started, so this does not make much sense and suggests you don't really have a valid mental model for threads.

Now, concerning notify_cancelled, you have two (invalid) options:

1. either you simply set stopped to True, which can only have an effect when the thread goes through your if not self.stopped statements, so it won't interrupt the processing: you don't want that.

2. either you also self.thread.exit(), but this is can interrupt both the processing and the upload... and you don't want that either.

What I would do is having a processing thread and an uploading thread: you could exit() the first one, but not the other one.