Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exceptions in DownloadChapterThread.run are being silenced #82

Closed
CharlieCorner opened this issue Oct 11, 2016 · 2 comments
Closed

Exceptions in DownloadChapterThread.run are being silenced #82

CharlieCorner opened this issue Oct 11, 2016 · 2 comments
Labels

Comments

@CharlieCorner
Copy link
Contributor

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
Copy link
Owner

jklmli commented Oct 11, 2016

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
Copy link
Contributor Author

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

jklmli added a commit that referenced this issue Dec 29, 2016
Fix #82: Avoid silencing underlying exceptions in DownloadChapterThread.run
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants