Fix: Reset stream on retries #285
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
product-auto-label
bot
added
the
api: storage
google-cla
bot
added
the
cla: no
|
@googlebot I signed it! |
google-cla
bot
added
cla: yes
fboismenu
changed the title
Fixes Issue googleapis#284. Reset Download _stream on retry to avoid accumulating data from previous failed request.
fboismenu
force-pushed
the
reset-stream-on-retry
branch
from
4f2054d
to
a8ee53f
Compare
There was a problem hiding this comment.
@fboismenu Thanks very much for the patch! In additional to my inline comments, we also need to address the following concerns:
- In order to demonstrate the fix, we need new unit tests which provoke retries after a write to the stream, and which assert that the stream does not contain that initial write after finishing the download.
- We need to consider how the fix generalizes to the other two classes (
ChunkedDownloadandRawChunkedDownload).
| def retriable_request(): | ||
|
|
||
| nonlocal is_retry |
There was a problem hiding this comment.
I'm pretty uncomfortable with nonlocal here -- I'd rather define / depend on an attribute of the Download instance.
|
|
||
| nonlocal is_retry | ||
| if is_retry and self._stream is not None: | ||
| self._stream.seek(0) |
There was a problem hiding this comment.
self._stream is documented only to be a "A write-able stream (i.e. file-like object)" -- we have no guarantee that it is seekable.
|
Hi, I'm working on submitting a fix, and will link the new PR back to this. Thanks! |
|
Superseded by #294 |
Fixes #284
Reset download stream before processing a retried request.
Avoids to accumulate data from a failed request.