shaka-packager icon indicating copy to clipboard operation
shaka-packager copied to clipboard

feat(HTTP): Retry failed HTTP requests with exponential backoff

Open mariocynicys opened this issue 1 year ago • 2 comments

This PR implements exponential backoff retrials for http files.

I am not quite familiar with the code base & how packager works, so I could use some help in clarifying the following:

  • In case of PUT or POST requests, the upload_cache_ would be drained, right? Does this mean we have to keep a copy of it?
  • Are there any other cases we should perform retries in? Other than client timeouts and these specific response code.

Tests yet to be added. Closes #1095

mariocynicys avatar Sep 09 '22 10:09 mariocynicys

Please merge with main, which has changed a lot in the recent cmake port. Thanks!

joeyparrish avatar Feb 08 '24 20:02 joeyparrish

This has been rebased to main and the tests do pass, but from some digging into the implications of download and upload cache I'm not sure whether this can be done safely. For example let's think of the case of using this to upload packaging output to some HTTP server.

As soon as CURL calls CurlReadCallback the underlying circular buffer is going to notify potential writers that were blocked on writing to the circular cache that there is room to write. And those writers may start writing right away, overwriting the data in the cache. Meanwhile the request fails and needs to be retried, but the underlying circular buffer was altered so the request cannot be retried.

I don't think this can be done safely with the currently IoCache interface as the backing buffers. I believe the data from the upload_cache would have to written to some other intermediary buffer (without triggering read events) so that curl and retry safely, and only upon success should the IoCache circular buffer signal the read event and advance the pointers.

Likewise for download cache, only upon successful download should the data be written to download_cache otherwise a consumer may have already consumed the bad response body by the time we retry.

Alternatively to avoid the memory copies I think we'd have to take a lock on IoCache around the curl_easy_perform loop and use some lower level interfaces than Read() and Write() (perhaps something like MaybeRead() and MaybeWrite()) and then explicitly release the lock and trigger read event once the request was successful.

cosmin avatar Feb 21 '24 06:02 cosmin