Fix race condition in GetObject that could result in empty responses #1334

passaro · 2025-03-26T06:41:15Z

Address an issue in the Stream implementation for S3GetObjectResponse that could immediately return None (i.e. terminate the stream) when detecting that the meta request had completed, before returning previously received parts. Reported in #1331.

The fix changes the mechanism used to extract the response body parts and the request completion from the meta request callbacks. Instead of multiple independent channels, it introduces a single channel that supports multiple S3GetObjectEvents. The events in the new channel match the order in which the callbacks are invoked, which is guaranteed by the CRT. The events channel also includes the Headers event, avoiding the need of a separate channel to await for the headers to be returned.

When using Mountpoint, an occurrence of this issue would result in a read request failing with an Input/output error, with a warning entry in the logs containing this message:

mountpoint_s3_fs::fuse: read failed with errno 5: get request failed: get request terminated unexpectedly

Note however that we were not able to trigger the issue in our tests.

Does this change impact existing behavior?

No.

Does this change need a changelog entry? Does it require a version change?

Bug fix entry and increase patch version.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

IsaevIlya · 2025-03-26T10:45:00Z

mountpoint-s3-client/src/s3_crt_client/put_object.rs

-        // Guaranteed when select_biased! executes the CreateMPU branch.
-        assert!(!request.is_terminated());
+        // Wait for CreateMultipartUpload to complete, or return error.
+        mpu_created.await.unwrap()?;


Would we wait indefinitely for mpu_created here? Is there any timeout under the hood or knowledge about CreateMultipartUpload that assure us that we won't wait forever?

See from line 93 above: if we get a result instead of the CreateMultipartUpload completing, it will be an error and we send it to this channel.

unexge · 2025-03-27T16:49:48Z

mountpoint-s3-client/src/s3_crt_client/get_object.rs

+            Some(S3GetObjectEvent::Error(e)) => {
+                return Err(e);
+            }
+            _ => {


Would it be useful to trace! the body here for debugging purpuses?

unexge · 2025-03-27T16:52:00Z

mountpoint-s3-client/src/s3_crt_client/get_object.rs

+                *this.next_offset = offset + part.len() as u64;
+                Poll::Ready(Some(Ok((offset, part))))
+            }
+            Poll::Ready(Some(S3GetObjectEvent::Headers(_))) => Poll::Pending,


This shouldn't ever happen right? Maybe we can emit some error log or assert

In fact, it could happen before. I've now updated the on_headers callback to only send the headers once and added an assertion here.

mountpoint-s3-client/src/s3_crt_client.rs

unexge · 2025-03-27T17:02:30Z

mountpoint-s3-client/src/s3_crt_client/put_object.rs

+                |_, _| {},
+                |_| None,
+                move |result| {
+                    if let Some(sender) = on_failure_sender.lock().unwrap().take() {


Maybe using an atomic bool instead of Arc<Mutex<Option<...>>> could be simpler here

Not sure what you have in mind.

Another approach I considered was to introduce an "event" channel like in get_object, but for MpuCreated and Result. However, it would require more changes to S3PutObjectRequest and we are considering removing the wait for CreateMultipartUpload anyway, so I decided to go for the less intrusive change.

I was thinking of having only one channel for all results and keeping track of errors in a AtomicBool, but I think that way the initial CreateMultipartUploadFailed error would be emitted once the caller first polls this method rather than initially from this method.

unexge · 2025-03-28T15:56:23Z

mountpoint-s3-client/tests/get_object.rs

+
+#[ignore = "Stress-test to run many concurrent GetObjects. To be run manually."]
+#[tokio::test(flavor = "multi_thread", worker_threads = 100)]
+async fn stress_test_get_object() {


unexge · 2025-03-28T16:04:04Z

mountpoint-s3-client/src/s3_crt_client/put_object.rs

+                |_, _| {},
+                |_| None,
+                move |result| {
+                    if let Some(sender) = on_failure_sender.lock().unwrap().take() {


I was thinking of having only one channel for all results and keeping track of errors in a AtomicBool, but I think that way the initial CreateMultipartUploadFailed error would be emitted once the caller first polls this method rather than initially from this method.

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

passaro · 2025-03-31T09:44:27Z

I have rebased and taken out the make_meta_request refactor. Now this change only includes the race condition fix. I'll open a separate PR for the other changes.

adpeace

lgtm, one minor comment about an inaccurate comment that can be addressed separately.

adpeace · 2025-03-31T14:27:37Z

mountpoint-s3-client/src/s3_crt_client.rs

    ) -> Result<S3HttpRequest<Vec<u8>, E>, S3RequestError> {
        let options = Self::new_meta_request_options(message, operation);
-        self.make_simple_http_request_from_options(options, request_span, |_| {}, on_error, |_, _| ())
+        self.make_simple_http_request_from_options(options, request_span, |_| {}, parse_meta_request_error, |_, _| ())
    }

    /// Make an HTTP request using this S3 client that returns the body on success or invokes the


Please can we update this comment? Looks like the interface of this function has changed slight (we seem to always send body to the channel, and parse errors through the caller-provided function).

(sounds like this is to be addressed in a later PR already in progress..)

passaro had a problem deploying to PR integration tests March 26, 2025 06:41 — with GitHub Actions Failure

passaro force-pushed the get-object-fix branch from d592d74 to 9129b0d Compare March 26, 2025 08:06

passaro temporarily deployed to PR integration tests March 26, 2025 08:06 — with GitHub Actions Inactive

passaro temporarily deployed to PR integration tests March 26, 2025 08:07 — with GitHub Actions Inactive

IsaevIlya reviewed Mar 26, 2025

View reviewed changes

IsaevIlya previously approved these changes Mar 27, 2025

View reviewed changes

passaro marked this pull request as ready for review March 27, 2025 16:49

unexge reviewed Mar 27, 2025

View reviewed changes

passaro dismissed IsaevIlya’s stale review via 04a1cbb March 28, 2025 09:06

passaro temporarily deployed to PR integration tests March 28, 2025 09:07 — with GitHub Actions Inactive

unexge previously approved these changes Mar 28, 2025

View reviewed changes

passaro force-pushed the get-object-fix branch from 04a1cbb to bbd0223 Compare March 28, 2025 19:40

passaro temporarily deployed to PR integration tests March 28, 2025 19:41 — with GitHub Actions Inactive

passaro added 3 commits March 31, 2025 09:21

Add (ignored) stress test

460a909

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

Fix race condition in GetObject

f83b2aa

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

Increment client version and update changelog

d5638dc

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

passaro dismissed unexge’s stale review via d5638dc March 31, 2025 09:31

passaro force-pushed the get-object-fix branch from bbd0223 to d5638dc Compare March 31, 2025 09:31

passaro temporarily deployed to PR integration tests March 31, 2025 09:31 — with GitHub Actions Inactive

unexge approved these changes Mar 31, 2025

View reviewed changes

adpeace reviewed Mar 31, 2025

View reviewed changes

adpeace approved these changes Mar 31, 2025

View reviewed changes

passaro added this pull request to the merge queue Mar 31, 2025

Merged via the queue into awslabs:main with commit a3909e0 Mar 31, 2025
26 checks passed

passaro deleted the get-object-fix branch March 31, 2025 16:01

passaro mentioned this pull request Mar 31, 2025

Refactor internal S3 client methods to initiate meta-requests #1337

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race condition in GetObject that could result in empty responses #1334

Fix race condition in GetObject that could result in empty responses #1334

passaro commented Mar 26, 2025 •

edited

Loading

IsaevIlya Mar 26, 2025

passaro Mar 26, 2025

unexge Mar 27, 2025

unexge Mar 27, 2025

passaro Mar 28, 2025

unexge Mar 27, 2025

passaro Mar 28, 2025

unexge Mar 28, 2025

unexge Mar 28, 2025

unexge Mar 28, 2025

passaro commented Mar 31, 2025

adpeace left a comment

adpeace Mar 31, 2025

adpeace Mar 31, 2025

Fix race condition in GetObject that could result in empty responses #1334

Fix race condition in GetObject that could result in empty responses #1334

Conversation

passaro commented Mar 26, 2025 • edited Loading

Does this change impact existing behavior?

Does this change need a changelog entry? Does it require a version change?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

passaro commented Mar 31, 2025

adpeace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

passaro commented Mar 26, 2025 •

edited

Loading