spl0k / supysonic

Supysonic is a Python implementation of the Subsonic server API.
https://supysonic.readthedocs.io
GNU Affero General Public License v3.0
260 stars 57 forks source link

Fix exception handling #201

Closed pR0Ps closed 3 years ago

pR0Ps commented 3 years ago

Bare excepts will catch GeneratorExit exceptions which are raised whenever a generator stops. This was causing issues when transcoding and caching the results.

All instances of bare excepts have been replaced with scoped versions.

codecov[bot] commented 3 years ago

Codecov Report

Merging #201 (bc6e768) into master (d6c00e0) will decrease coverage by 0.11%. The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage   87.15%   87.03%   -0.12%     
==========================================
  Files          39       39              
  Lines        3588     3594       +6     
==========================================
+ Hits         3127     3128       +1     
- Misses        461      466       +5     
Impacted Files Coverage Δ
supysonic/api/media.py 93.27% <0.00%> (-2.14%) :arrow_down:
supysonic/jukebox.py 25.21% <0.00%> (ø)
supysonic/api/__init__.py 98.76% <100.00%> (ø)
supysonic/cache.py 99.15% <100.00%> (+<0.01%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d6c00e0...bc6e768. Read the comment docs.

spl0k commented 3 years ago

What kind of issue did you notice with transcoding and caching? Is it related to #202? Handling the GeneratorExit in transcode() might be necessary in order to properly stop processes if a client prematurely close the connection.

pR0Ps commented 3 years ago

I was seeing that transcodes wouldn't ever be cached, even without estimating the content length.

This was because when finishing a transcode, the GeneratorExit exception was always being raised by transcode, handled, and re-raised. This re-raising caused it to bubble up through to the set_generated and set_fileobj functions of the cache, and trigger the exception handling there which deleted the now-fully transcoded file. #202 was raised because in investigating this, I realized that Jamstash was always closing the connection early when estimation was turned on, causing nothing to be cached.

It looks like the change to the cache exception handling was made here.

You're right about the GeneratorExit being needed. I didn't see this issue while testing it last night, but I just skipped around wildly, let some streams finish loading, and togged the estimateContentLength param on and off in Jamstash in a few different browsers and managed to get 2 transcode processes stuck. No idea why it's so intermittent, but I've changed that one back to killing the processes on all exceptions including GeneratorExit.

I don't think the exception hander in set_fileobj should trigger on GeneratorExit since that would cause the file to always be deleted even if the transcode generator finishes normally. It makes sense to handle GeneratorExit while transcoding since the processes always have to be cleaned up, but the exception handler in set_fileobj should only be handling actual errors.