Skip to content

Conversation

@timostrunk
Copy link
Contributor

@timostrunk timostrunk commented Aug 18, 2023

Notes

This is equivalent to cherry-picking fd2759aa16b12b33298900c77d29b3813c6582de onto the current vendored version 1.26.15. We are merging the cherry pick to unblock release.

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-739316: Snowflake Batch Results (get_result_batches()) #1425

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am MAYBE modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

urllib3 does not do content-length checking.
result_batch.py does a get request in line 298, which can therefore in case of connection issues return an incomplete response. This is passed directly to an ArrowIterator. The already existing retry mechanism does not catch this as no Exception is generated.

Please write a short description of how your code change solves the related issue.
4. Setting enforce_content_length checks the length of the response and raises an IncompleteRead error, which should already trigger the existing retry mechanism.
This is the same behaviour as in urllib 3 V2.0.

In other words: #1689 is the better fix to this problem. I'm sending this PR to document the issue and maybe to provide a quickfix version for the interested parties.

Also some info: I should add that this is not confirmed to fix the issue yet. It's just my best guess. I also thought about actually not only overriding the default, but also enforcing enforce_content_length to true in the function itself. But I think requests is the only user in this case and it's also vendored and it never sets it explicitly, so I just overrode the default.

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@timostrunk
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@timostrunk
Copy link
Contributor Author

recheck

@timostrunk timostrunk force-pushed the patch_vendored_urllib branch from c5dd73f to 910762b Compare August 18, 2023 14:49
@timostrunk timostrunk force-pushed the patch_vendored_urllib branch from 910762b to a46c820 Compare August 18, 2023 14:51
@sfc-gh-stan sfc-gh-stan changed the title enforce content-length checking in vendored urllib. Fixes #1425 SNOW-739316 Enforce content-length checking in vendored urllib3 Aug 21, 2023
@sfc-gh-stan sfc-gh-stan merged commit 02cf139 into snowflakedb:main Aug 25, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SNOW-739316: Snowflake Batch Results (get_result_batches())

2 participants