Skip to content

Conversation

NoahStapp
Copy link
Contributor

No description provided.

@NoahStapp NoahStapp requested a review from blink1073 October 9, 2024 17:59
@@ -145,6 +145,8 @@ def _is_ready(fut: Future) -> None:
read = conn.recv_into(mv[total_read:])
if read == 0:
raise OSError("connection closed")
if once:
Copy link
Member

Choose a reason for hiding this comment

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

Why only once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to support KMS requests, which require that the read returns the first batch immediately to accurately update the expected length.

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't sound right. If we know how much we need to read we should be looping until we read that amount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This loop inside _async_kms_request changes kms_context.bytes_needed with each recv and feed call. If we allow async_receive_data_socket to run until the initial kms_context.bytes_needed number of bytes is read, it will result in an error.

while kms_context.bytes_needed > 0:
    # CSOT: update timeout.
    conn.settimeout(max(_csot.clamp_remaining(_KMS_CONNECT_TIMEOUT), 0))
    data = await async_receive_data_socket(conn, kms_context.bytes_needed)
    kms_context.feed(data)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the behavior I observed that led to this change:

kms_context.bytes_needed = N
async_receive_data_socket returns with X bytes read, X < N
kms_context.feed is called
kms_context.bytes_needed is now not N - X, but a different value

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, looks like that's intentional behavior in libmongocrypt:

/* Before we've read the Content-Length header in an HTTP response,
 * we don't know how many bytes we'll need. So return this value
 * in kms_ctx_bytes_needed until we are fed the Content-Length.
 */
#define DEFAULT_MAX_KMS_BYTE_REQUEST 1024

https://github.com/mongodb/libmongocrypt/blob/7aeaec4/src/mongocrypt-kms-ctx.c#L47-L51

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's a little odd but this approach allows us to reuse as much code as possible.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add Shane's note as a comment for future readers?

@NoahStapp
Copy link
Contributor Author

@blink1073
Copy link
Member

Maybe we need to skip that test and mark it for PYTHON-4493?

@ShaneHarvey
Copy link
Member

Agree. I suggest opening a new ticket for the skipped test to make sure it doesn't get lost.

@NoahStapp
Copy link
Contributor Author

test.asynchronous.test_encryption.AsyncTestSpec.test_legacy_timeoutMS_timeoutMS_applied_to_listCollections_to_get_collection_schema

Skipped in #1914.

@blink1073
Copy link
Member

Somehow that test is still failing.

@NoahStapp
Copy link
Contributor Author

Somehow that test is still failing.

I messed up the merge, fixed now.

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

@NoahStapp NoahStapp merged commit 3a66229 into mongodb:master Oct 11, 2024
36 checks passed
@NoahStapp NoahStapp deleted the PYTHON-4700 branch October 11, 2024 14:48
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