B2-40 support 100 Continue #300

emnoor-reef · 2023-08-09T14:43:38Z

The b2sdk/urllib3/_connection.py file is mostly copied from botocore. I have updated at some places to support urllib3>=2 (botocore pins urllib3<1.27).

mjurbanski-reef

I like reusing the opensource components here 💪
this is why I left most of the comments on License-related files - nitpicks mostly, but lets make sure we are playing by the rules correctly :)

b2sdk/requests/NOTICE

b2sdk/urllib3/NOTICE

b2sdk/urllib3/included_source_meta.py

b2sdk/requests/included_source_meta.py

b2sdk/urllib3/_connection.py

mjurbanski-reef · 2023-08-12T19:14:15Z

b2sdk/urllib3/NOTICE

+The following classes have been overriden for support HTTP 100-continue:
+urllib3.connection.HTTPConnection, urllib3.connection.VerifiedHTTPSConnection,
+urllib3.connectionpool.HTTPConnectionPool, urllib3.connectionpool.HTTPSConnectionPool


these are not our (Backblaze) changes as you are suggesting thru the header above!

IMO we should just say we are vendoring part of botocore that is relavant to supporting 100 Continue and list our changes to it below it.

b2sdk/b2http.py

test/integration/test_raw_api.py

mjurbanski-reef · 2023-08-12T19:37:09Z

b2sdk/urllib3/_connection.py

+            # Wait for 1 second for the server to send a response.
+            if urllib3.util.wait_for_read(self.sock, 2):


lol, this is a good case showing why putting an obvious is decremental
it says 1 second, but wait is actually 2 ;p

as to why I'm commenting on this: we may need this to be user-configurable
https://github.com/reef-technologies/b2-sdk-python/pull/252/files#diff-ac825af986ae146afcaa98597de5a9c07399afb45b45edbba3d2b7ab68db124eR26

Please for now just expose it so it is controllable for HTTPAdapterWithContinue user and use 10 as default since that value was chosen with pycurl for some reason.

mjurbanski-reef · 2023-08-12T19:41:38Z

test/integration/test_raw_api.py

+            headers.update({
+                'Expect': '100-continue',
+            })


wait, what, we are not setting this header by default during upload etc?
I wonder what is the purpose of adding this in if we are not using it

please consult this with @pawelpolewicz if this should be enabled by default or turned off completely for now

I'm leaving out the "optional" option since IMO having it is for "greater good" (infrastructure wise ;) ) and having it always on may only negatively affect very small file uploads which are not the main use case of b2 AFAIK.

Also added two configurations in B2HttpApiConfig to disable this feature or to configure 100-continue response timeout

mjurbanski-reef · 2023-08-13T19:40:36Z

b2sdk/_botocore/awsrequest.py

@@ -82,6 +83,11 @@ def request(self, method, url, body=None, headers=None, *args, **kwargs):
        self._response_received = False
        if headers.get('Expect', b'') in [b'100-continue', '100-continue']:
            self._expect_header_set = True
+            timeout = headers.pop('X-Expect-100-Continue-Timeout-Seconds', self._continue_timeout)


nitpick: drop -Seconds part of the name; AFAIK there are already timeout settigns http headers, none of which adds -seconds (Cache-Control, Keep Alive etc)

mjurbanski-reef · 2023-08-13T19:42:11Z

b2sdk/_botocore/awsrequest.py

+            except (ValueError, TypeError):
+                pass


personally, I would prefer it explode; name collision seems unlikely for it to be breaking change

mjurbanski-reef

solution looks good now, but I'm pretty sure there is still something shady going on in the test

also make sure default is set properly & add it to changelog please

mjurbanski-reef · 2023-08-13T19:47:38Z

b2sdk/api_config.py

-        decode_content: bool = False
+        decode_content: bool = False,
+        expect_100_continue: bool = True,
+        expect_100_continue_timeout_seconds: float = 10.0,


please rename to expect_100_timeout across the whole code base and make sure you added it to docstrings to methods which already have params in docstrings

Lets not do "expect 100 continue timeout" in one place and "expect 100 timeout" in another. Lets have one and stick with it.

mjurbanski-reef · 2023-08-13T19:49:08Z

b2sdk/file_version.py

@@ -351,6 +351,7 @@ def _get_upload_headers(self) -> bytes:
            file_retention=self.file_retention,
            legal_hold=self.legal_hold,
            cache_control=self.cache_control,
+            expect_100_continue=False,


wasn't it supposed to be ON by default with option of disabling?

Isn't file_version a separate request than an upload request?

sorry my bad:)

also this isn't used in any request but in has_large_header from what I see

mjurbanski-reef · 2023-08-13T19:52:10Z

NOTES

@@ -0,0 +1,5 @@
+response status line = b'HTTP/1.1 401 \r\n'


mjurbanski-reef · 2023-08-14T06:13:18Z

b2sdk/_botocore/awsrequest.py

+            # This is our custom behavior.  If the Expect header was
+            # set, it will trigger this custom behavior.
+            logger.debug("Waiting for 100 Continue response.")
+            # Wait for 10 seconds for the server to send a response.


remove this comment :) it is not accurate depending on configuration

mjurbanski-reef · 2023-08-14T06:23:45Z

b2sdk/file_version.py

@@ -351,6 +351,7 @@ def _get_upload_headers(self) -> bytes:
            file_retention=self.file_retention,
            legal_hold=self.legal_hold,
            cache_control=self.cache_control,
+            expect_100_continue=False,


sorry my bad:)

also this isn't used in any request but in has_large_header from what I see

emnoor-reef · 2023-08-15T13:45:50Z

I agree the test is a bit shady.

In the last commit I moved the test into a separate file. I added two more tests for the the configurations. Though I am still not happy with the tests.

Also, the fixture could be broken up into reusable parts. Currently it borrows all of it's code form test_raw_api. But I think that could be refactored with test_raw_api when that is done.

mjurbanski-reef · 2023-08-17T20:25:05Z

test/integration/test_raw_expect_100.py

+@pytest.fixture(autouse=True)
+def rest_sent_data(expect_100_setup):
+    expect_100_setup["sent_data"].clear()


it kinda seems to me like this test will work even if this was deleted

mjurbanski-reef · 2023-08-17T20:29:02Z

test/integration/test_raw_expect_100.py

+        "raw_api": raw_api,
+        "upload_url": upload_url,
+        "upload_auth_token": upload_auth_token,
+        "sse_b2_aes": sse_b2_aes,
+        "sent_data": sent_data,
+        "file_name": file_name,
+        "file_contents": file_contents,
+        "file_length": file_length,
+        "file_sha1": file_sha1,


seems like you crammed bunch of fixtures into single one
this doesn't encourage reusability

mjurbanski-reef · 2023-08-17T20:38:20Z

b2sdk/raw_api.py

+        :param expect_100_continue: whether to use 'Expect: 100-continue' header
+        :param expect_100_timeout: timeout of 100 response when expect_100_continue is True


sorry for another change here, but now that I look at it - wouldn't expect_100_timeout being set to 0 be basically the same think and we could skip adding expect_100_continue param altogether?

mjurbanski-reef · 2023-08-17T20:40:32Z

test/integration/test_raw_expect_100.py

+    assert expect_100_setup["file_contents"] not in expect_100_setup["sent_data"]
+
+
+def test_expect_100_timeout(expect_100_setup):


and where is the test that we actually don't send data before 100 Continue is received from the server, but after we get it, we do send data? That is the most important test; The "ignoring 100 Continue" is what we did so far without modification to urrlib&requests

mjurbanski-reef · 2023-08-17T20:46:08Z

test/integration/test_raw_expect_100.py

+    bucket_dict = raw_api.create_bucket(
+        api_url,
+        account_auth_token,
+        account_id,
+        bucket_name,
+        'allPublic',
+        is_file_lock_enabled=True,
+    )


please reuse the single bucket for all tests here - there is not reason not to, and it will reduce the overall API calls i.e. make tests faster

…python into 100-continue * '100-continue' of https://github.com/emnoor-reef/b2-sdk-python: Fix failing test Fix file headers Fix formatting

test/integration/test_raw_expect_100.py

mjurbanski-reef

https://github.com/reef-technologies/b2-sdk-python/pull/300/files#r1297715473 would be nice to include, but not exactly crucial

test/unit/botocore/test_awsrequest.py

mjurbanski-reef · 2023-09-18T19:36:21Z

test/integration/test_raw_expect_100.py

+        )
+    assert file_contents in http_sent_data
+    assert wait_mock.call_count == 1
+    args, _ = wait_mock.call_args


whats the purpose of this?

forgot to remove that line :}

test/unit/botocore/test_awsrequest.py

draft support for 100-continue through stdlib http.client

099294c

mjurbanski-reef changed the base branch from master to AddApiToFolder August 9, 2023 15:00

mjurbanski-reef changed the base branch from AddApiToFolder to master August 9, 2023 15:00

emnoor-reef added 7 commits August 12, 2023 21:56

fix header type when checking for Expect: 100-continue

cee2665

Handle b2 server responding with empty text phrase in status line

b72b838

Update NOTICE file to include urllib3 changes

f2c2bde

Update test for 100-continue

08c4f16

Add test for 100-continue success case

56a741e

Format code

9b7f80c

Move urllib3 subclasses into separate package

334ef68

emnoor-reef marked this pull request as ready for review August 12, 2023 16:45

emnoor-reef changed the title ~~100 continue~~ B2-40 support 100 Continue Aug 12, 2023

mjurbanski-reef requested changes Aug 12, 2023

View reviewed changes

emnoor-reef added 8 commits August 13, 2023 20:36

Fix NOTICE of vendored requests

3c23b92

Move HTTPAdapterWithContinue to b2sdk.requests

3c1c10c

Fix botocore vendor package name & NOTICE

b0e4063

Update botocore LICENCE & NOTICE

456b93b

Add urllib3 as explicit dependency

2d905b5

Add file contents inclusion assertion to 100-continue test

73b7237

Enable 100-continue for file uploads by default

3c2aa62

Also added two configurations in B2HttpApiConfig to disable this feature or to configure 100-continue response timeout

Fix broken b2sdk/file_version.py

dd1ecd5

mjurbanski-reef reviewed Aug 13, 2023

View reviewed changes

mjurbanski-reef requested changes Aug 13, 2023

View reviewed changes

mjurbanski-reef reviewed Aug 13, 2023

View reviewed changes

NOTES Outdated

@@ -0,0 +1,5 @@

response status line = b'HTTP/1.1 401 \r\n'

Copy link

mjurbanski-reef Aug 13, 2023

Choose a reason for hiding this comment

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

🗑️

emnoor-reef added 2 commits August 14, 2023 02:27

Propagate api_config.expect_100_* settings

42a6fc1

Rename except-100 vars & headers

5009d6e

mjurbanski-reef reviewed Aug 14, 2023

View reviewed changes

emnoor-reef added 2 commits August 15, 2023 17:59

Remove misleading comment

710c361

Add changelog entry for 100-continue

9c92658

Move expect-100 tests into separate file

015eb4d

emnoor-reef added 3 commits August 16, 2023 00:45

Fix formatting

1ed88bb

Fix file headers

2e53be1

Fix failing test

4478dd2

mjurbanski-reef requested changes Aug 17, 2023

View reviewed changes

emnoor-reef added 6 commits September 2, 2023 23:21

Fix typos

28b4d40

Separate fixtures from big expect_100_setup fixture

c55f813

Fix lint issues

504b3f7

Merge branch '100-continue' of https://github.com/emnoor-reef/b2-sdk-…

ed9a201

…python into 100-continue * '100-continue' of https://github.com/emnoor-reef/b2-sdk-python: Fix failing test Fix file headers Fix formatting

add the unit tests from botocore's AWSHTTPConnection

42bc4ab

Update botocore notice about its unittest

5819641

mjurbanski-reef reviewed Sep 18, 2023

View reviewed changes

test/integration/test_raw_expect_100.py Show resolved Hide resolved

emnoor-reef added 3 commits September 19, 2023 00:53

check for waiting in 100 continue integration tests

b54bb3a

add test to verify data is sent AFTER waiting for 100 Continue response

5866262

fix patched wait_for_read in test

2f337cf

mjurbanski-reef approved these changes Sep 18, 2023

View reviewed changes

remove unnecessary tests for 100 continue

abde2c1

emnoor-reef requested a review from mjurbanski-reef September 18, 2023 19:56

emnoor-reef closed this by deleting the head repository Feb 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

B2-40 support 100 Continue #300

B2-40 support 100 Continue #300

emnoor-reef commented Aug 9, 2023 •

edited

Loading

mjurbanski-reef left a comment

mjurbanski-reef Aug 12, 2023

mjurbanski-reef Aug 12, 2023

mjurbanski-reef Aug 12, 2023

mjurbanski-reef Aug 13, 2023

mjurbanski-reef Aug 13, 2023

mjurbanski-reef left a comment

mjurbanski-reef Aug 13, 2023

mjurbanski-reef Aug 13, 2023

emnoor-reef Aug 13, 2023

mjurbanski-reef Aug 14, 2023

mjurbanski-reef Aug 13, 2023

mjurbanski-reef Aug 14, 2023

mjurbanski-reef Aug 14, 2023

emnoor-reef commented Aug 15, 2023

mjurbanski-reef Aug 17, 2023

mjurbanski-reef Aug 17, 2023

mjurbanski-reef Aug 17, 2023

mjurbanski-reef Aug 17, 2023

mjurbanski-reef Aug 17, 2023

mjurbanski-reef left a comment

mjurbanski-reef Sep 18, 2023

emnoor-reef Sep 18, 2023

		# Wait for 1 second for the server to send a response.
		if urllib3.util.wait_for_read(self.sock, 2):

		:param expect_100_continue: whether to use 'Expect: 100-continue' header
		:param expect_100_timeout: timeout of 100 response when expect_100_continue is True

		assert expect_100_setup["file_contents"] not in expect_100_setup["sent_data"]


		def test_expect_100_timeout(expect_100_setup):

B2-40 support 100 Continue #300

B2-40 support 100 Continue #300

Conversation

emnoor-reef commented Aug 9, 2023 • edited Loading

mjurbanski-reef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjurbanski-reef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emnoor-reef commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjurbanski-reef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emnoor-reef commented Aug 9, 2023 •

edited

Loading