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 a timeout error on Windows when writing to a S3 bucket #141

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edouarda
Copy link

@edouarda edouarda commented Sep 5, 2021

  • Binary files must be opened with the binary flag or reading may stop
    before EOF, causing a timeout as AWS waits for the end of the file
  • Added an #undef for GetFreeSpace to fix a compilation error on
    Windows
  • Added default definition when USE_AWS is 0
  • Fixed integer cast
  • Fixed uninitialized variable warning
  • Fixed include

 * Binary files must be opened with the binary flag or reading may stop
   before EOF, causing a timeout as AWS waits for the end of the file
 * Added an #undef for GetFreeSpace to fix a compilation error on
   Windows
 * Added default definition when USE_AWS is 0
 * Fixed integer cast
 * Fixed uninitialized variable warning
 * Fixed include
@dhruba
Copy link

dhruba commented Sep 18, 2021

@mrambacher This is a very interesting fix because it uses "std::ios_base::binary". Can you pl review this one and then do the needful? thanks.

Copy link
Collaborator

@mrambacher mrambacher left a comment

Choose a reason for hiding this comment

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

This looks fine to me. I am not sure I understand why/where the timeout failure on Windows is, so it might be nice to add a comment somewhere (even in the PR) explaining the fix and why it wors.

#ifdef _WIN32_WINNT
#undef GetFreeSpace
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

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

Other places seem to undef this based on it being defined, not based on Windows. Can we follow the same pattern?

long percent = (capacity > 0 ? (100L * usage / capacity) : 0);
auto percent = (capacity > 0 ? (100L * usage / capacity) : 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the change to auto?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants