search

shaka-project / shaka-packager

A media packaging and development framework for VOD and Live DASH and HLS applications, supporting Common Encryption for Widevine and other DRM Systems.
https://shaka-project.github.io/shaka-packager/
Other
1.9k stars 496 forks source link

fix: http_file: Close upload cache on task exit #1348

Closed petzeb closed 4 months ago

petzeb commented 4 months ago

In some cases it can happen that the http server responds with a non-successful status code without reading the response body. In this case curl may decide not to read from the cache since there is really no point in sending data to the server. In case some other thread of shaka has already called HttpFile::Flush it may end up deadlocked there waiting for the cache to either close or become empty. Thus, we close the cache when leaving the main thread as no data will be read by curl after it has finished anyways.

Closes #1347

petzeb commented 4 months ago

This attempts to fix: https://github.com/shaka-project/shaka-packager/issues/1347

The problem appears to be that the ThreadedIoFile that wraps HttpFile calls Flush() in its Close() method. If this happens and curl decides not to empty the upload_cache_ in HttpFile then it will hang there for ever.

An alternative approach to the one I have drafted here could be to add a call to CloseForWriting() somewhere around ThreadedIoFile::Close().

I tried to add a unit test for this, but struggled to make it reproduce. I am thinking that the embedded server used in the unit tests end up with different enough behavior that curl has enough time to read and send the payload before the server responds with 403. Help appreciated!

joeyparrish commented 4 months ago

This seems reasonable to me. @cosmin, since you were looking at details of the HTTP upload implementation recently for another PR, can you comment on this one?