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

Fix corrupted S3 downloads when file object in append mode #112

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions s3transfer/compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,20 @@ def accepts_kwargs(func):
MAXINT = sys.maxint


def writes_are_seekable(fileobj):
"""Backwards compat function to determine if writes to a fileobj are seekable

:param fileobj: The file-like object to determine if writes are seekable

:returns: True, if in append mode. False, otherwise.
"""
APPEND_ONLY_MODE = 'a'
is_seekable = seekable(fileobj)
if hasattr(fileobj, 'mode') and APPEND_ONLY_MODE in fileobj.mode:
is_seekable = False
return is_seekable


def seekable(fileobj):
"""Backwards compat function to determine if a fileobj is seekable

Expand Down
4 changes: 2 additions & 2 deletions s3transfer/download.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
ReadTimeoutError

from s3transfer.compat import SOCKET_ERROR
from s3transfer.compat import seekable
from s3transfer.compat import seekable, writes_are_seekable
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a fly-by comment here, but unless I'm missing something, you don't actually call this writes_are_seekable() function anywhere, so how does this fix the problem?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent that someone is awake, added the missing call from my staging 👍

from s3transfer.exceptions import RetriesExceededError
from s3transfer.futures import IN_MEMORY_DOWNLOAD_TAG
from s3transfer.utils import random_file_extension
Expand Down Expand Up @@ -202,7 +202,7 @@ def _get_temp_fileobj(self):
class DownloadSeekableOutputManager(DownloadOutputManager):
@classmethod
def is_compatible(cls, download_target, osutil):
return seekable(download_target)
return writes_are_seekable(download_target)

def get_fileobj_for_io_writes(self, transfer_future):
# Return the fileobj provided to the future.
Expand Down
8 changes: 7 additions & 1 deletion tests/unit/test_compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from botocore.compat import six

from tests import unittest
from s3transfer.compat import seekable, readable
from s3transfer.compat import seekable, readable, writes_are_seekable


class ErrorRaisingSeekWrapper(object):
Expand Down Expand Up @@ -49,6 +49,12 @@ def test_seekable_fileobj(self):
with open(self.filename, 'w') as f:
self.assertTrue(seekable(f))

def test_non_seekable_append_fileobj(self):
with open(self.filename, 'a') as f:
self.assertFalse(writes_are_seekable(f))
with open(self.filename, 'a+') as f:
self.assertFalse(writes_are_seekable(f))

def test_non_file_like_obj(self):
# Fails becase there is no seekable(), seek(), nor tell()
self.assertFalse(seekable(object()))
Expand Down