jklmli / manga_downloader

Cross-platform, multi-site, multi-threaded manga downloader with over 5000 distinct mangas. Includes support for automated downloading via external .xml file and conversion for viewing on the Kindle.
MIT License
270 stars 53 forks source link

Exceptions in DownloadChapterThread.run are being silenced #82

Closed CharlieCorner closed 7 years ago

CharlieCorner commented 8 years ago

Recently #81 was raised to troubleshoot a problem when downloading chapters, but the Tracebacks that were shared didn't have enough information to pinpoint exactly where the code actually broke and why.

This is because in base.py lines 391-403 of the DownloadChapterThread class we are re-raising a FatalError with just a short message but not preserving the original Traceback:

    def run (self):
        try:
            self.siteParser.processChapter(self, self.chapter)  
        except Exception as exception:
            # Assume semaphore has not been release
            # This assumption could be faulty if the error was thrown in the compression function
            # The worst case is that releasing the semaphore would allow one more thread to 
            # begin downloading than it should
            #
            # If the semaphore was not released before the exception, it could cause deadlock
            chapterThreadSemaphore.release()
            self.isThreadFailed = True
            raise FatalError("Thread crashed while downloading chapter: %s" % str(exception))

If an Exception is raised in any part of the underlying code in self.siteParse.processChapter(self, self.chapter) it will be silenced by the code on line 403 where we raise FatalError.

We need to find a way to either wrap the original exception properly in a FatalError, or just re-raise the original exception altogether so as not to lose our ability to hunt down bugs.

Unless I'm skipping something is raising our custom FatalError really giving us any value here?

jklmli commented 8 years ago

No, I agree that wrapping the exception diminishes the value instead of increasing it. I'd check to see if any code depends on FatalError, and - if not - remove the wrapping.

CharlieCorner commented 8 years ago

I mean, if we really want to keep the wrapping I think it is still doable.

Granted, in Python3 we have the raise-from statement and it'd be easier, but in Python2 we'd need to jump through a few hoops in order to implement it correctly using sys.exc_info, we just need to take into account the Warnings stated in the documentation:

https://docs.python.org/2/library/sys.html#sys.exc_info