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: warn streaming requests are not retryable #1092

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Dec 1, 2023

Current behavior when retrying a streaming payload:

  • client hangs, the stream has been read for the initial request and cannot be read again for the retry

In this PR:

  • a warning is logged: "An error was encountered in a non-retryable streaming request."
  • the error is thrown regardless of its retryable status

How to reproduce streaming retry:

import { S3 } from "@aws-sdk/client-s3";
import { NodeHttpHandler } from "@smithy/node-http-handler";
import { Readable } from "stream";

function streamToString(stream) {
  const chunks = [];
  return new Promise((resolve, reject) => {
    stream.on("data", (chunk) => chunks.push(Buffer.from(chunk)));
    stream.on("error", (err) => reject(err));
    stream.on("end", () => resolve(Buffer.concat(chunks).toString("utf8")));
  });
}

const s3 = new S3({
  region: "us-west-2",
  requestHandler: new (class {
    constructor() {
      this.call = 0;
      this.handler = new NodeHttpHandler({});
    }

    async handle(req) {
      if (this.call++ === 0) {
        const body = await streamToString(req.body);
        console.log("mock error has read the stream body", body);
        throw {
          $metadata: {
            httpStatusCode: 500,
          },
        };
      }
      return this.handler.handle(req);
    }
  })(),
});

const Bucket = "....";

await s3.putObject({
  Bucket,
  Key: "retry-stream",
  Body: Readable.from("abcd"),
  ContentLength: 4,
});

This PR prevents retries for streaming payloads because we cannot restore the stream.
A warning is also logged prior to the error.

@kuhe kuhe requested review from a team as code owners December 1, 2023 16:36
@kuhe kuhe requested a review from sugmanue December 1, 2023 16:36
@kuhe
Copy link
Contributor Author

kuhe commented Dec 1, 2023

fixes aws/aws-sdk-js-v3#5479

@kuhe kuhe force-pushed the fix/retry branch 3 times, most recently from 9a54d47 to 4468a30 Compare December 1, 2023 16:47
kuhe and others added 3 commits December 1, 2023 11:56
…yload.browser.ts

Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
…yload.ts

Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
@kuhe kuhe merged commit 44f78bd into smithy-lang:main Dec 1, 2023
7 checks passed
@kuhe kuhe deleted the fix/retry branch December 1, 2023 17:49
@vvo
Copy link

vvo commented Mar 5, 2024

@kuhe We're getting these warnings for unrelated errors, for example when trying to pass an UploadId that doesn't exist in an UploadPartCommand. I wonder if always sending the stream warning on any error is what we really want here?

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.

None yet

4 participants