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

feat(util-stream): implement aws-chunked-body #4787

Closed
wants to merge 9 commits into from

Conversation

milesziemer
Copy link
Contributor

@milesziemer milesziemer commented Jun 2, 2023

Issue

Issue number, if available, prefixed with "#"

Description

What does this implement/fix? Explain your changes.

Implements the getAwsChunkedBody function which converts an
AsyncIterable to the aws-chunked-body encoding stream format,
and will replace the getAwsChunkedEncodingStream function.
The method itself is runtime agnostic, but utility functions
readableToAsyncIterable and asyncIterableToReadable were added
to convert between AsyncIterable and the runtime stream type.
GetAwsChunkedEncodingStream was deprecated.

middlware-flexible-checksum, the only middleware to use the old
implementation was updated to allow callers to optionally provide
an implementation of GetAwsChunkedBody, which will be used instead
of the required GetAwsChunkedEncodingStream implementation.

The code generator was updated to use the util-stream package and
generate the getAwsChunkedBody config property when necessary,
and all the clients were regenerated.

A deprecation message was added to the readmes of util-stream-node
and util-stream-browser.

Testing

How was this change tested?

  • Tests were added to middleware-flexible-checksum to make sure the
    GetAwsChunkedBody is used when present, and
    GetAwsChunkedEncodingStream is used otherwise. Tests were also added
    to util-stream to test the functionality of getAwsChunkedBody. Karma
    was added to the package so getAwsChunkedBody can be tested in Node
    and browser runtimes.

  • yarn test:protocols and yarn test:integration:legacy

  • A test was set up using the aws-sdk-js-tests repo to call S3's GetObject
    using the updated code, to verify it works in both node and browser runtimes.

Additional context

Add any other context about the PR here.

This PR requires the codegen change in smithy-lang/smithy-typescript#775,
so CI will fail until it is merged. This PR will be converted from a draft when the smithy-typescript
PR is merged.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Updates the codegenerator to use the runtime-agnostic util-stream
package for getAwsChunkedEncodingStream.
Implements the getAwsChunkedBody function which converts an
AsyncIterable to the aws-chunked-body encoding stream format,
and will replace the getAwsChunkedEncodingStream function.
The method itself is runtime agnostic, but utility functions
readableToAsyncIterable and asyncIterableToReadable were added
to convert between AsyncIterable and the runtime stream type.
GetAwsChunkedEncodingStream was deprecated.

middlware-flexible-checksum, the only middleware to use the old
implementation was updated to allow callers to optionally provide
an implementation of GetAwsChunkedBody, which will be used instead
of the required GetAwsChunkedEncodingStream implementation.

Tests were added to middleware-flexible-checksum to make sure the
GetAwsChunkedBody is used when present, and
GetAwsChunkedEncodingStream is used otherwise. Tests were also added
to util-stream to test the functionality of getAwsChunkedBody. Karma
was added to the package so getAwsChunkedBody can be tested in Node
and browser runtimes.
Updates the code generator to use the new getAwsChunkedBody
implementation in the config generator. The function is runtime
agnostic so is added to the shared config. A deprecation message
was also added to getAwsChunkedEncodingStream in the config
interface.
Adds a deprecation notice to the READMEs of util-stream-node
and util-stream-browser.
This commit contains the changes to generated clients for the
AwsChunkedBody implementation.
@milesziemer milesziemer changed the title Impl aws chunked body feat(util-stream): implement aws-chunked-body Jun 2, 2023
readable.on("end", () => {
expect(mockStreamHasher).not.toHaveBeenCalled();
expect(mockBase64Encoder).not.toHaveBeenCalled();
expect(buffer).toEqual(`5\r\nHello\r\n5\r\nWorld\r\n0\r\n`);
Copy link
Contributor

Choose a reason for hiding this comment

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

for strings like these you could write it like

[
  '5',
  'Hello',
  '5',
  'World',
  '0',
].join('\r\n') + '\r\n'

to make them easier to read and edit, or break the \n into actual new lines in the template string

/**
* @internal
*/
export * from "./asyncIterableToReadable";
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the release level flags from re-exports, the original declaration will be the source of truth

export async function* readableToAsyncIterable<T>(readStream: Readable): AsyncIterable<T> {
let streamEnded = false;
let generationEnded = false;
const records = new Array<T>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const records = new Array<T>();
const records: T[] = [];

Copy link
Contributor

Choose a reason for hiding this comment

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

can this function be implemented without this queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the reason is the same as #4787 (comment), you can't read the stream chunk-by-chunk and yield, but I might be missing something


while (!generationEnded) {
// @ts-ignore TS2345: Argument of type 'T | undefined' is not assignable to type 'T | PromiseLike<T>'.
const value = await new Promise<T>((resolve) => setTimeout(() => resolve(records.shift()), 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like you intend to have the function wait one event loop, can this be... ?

await 0;
const next = records.shift();
if (next) {
  yield next;
}

proof of concept:

let x = async () => {
  console.log('start');
  await 0;
  console.log('end');
};
x(); x();

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 works

@milesziemer
Copy link
Contributor Author

Closing in favor of #4861. The implementation of getAwsChunkedBody in this PR doesn't work because streamHasher operates on ReadableStream in node runtime. We can move to AsyncIterable in the future, but switching clients to util-stream doesn't have to wait on that.

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 5, 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.

2 participants