Http Handlers #57

chrisradek · 2017-10-23T18:37:53Z

This is an early WIP. The stream collectors shouldn't change much, but the http handlers will still change. Currently working on http tests.

Posting this as a reference.

chrisradek · 2017-10-31T19:57:48Z

This PR has now been updated. I've tested the node http handler middleware on node.js, and the fetch handler middleware in chrome/firefox.

chrisradek · 2017-10-31T22:35:09Z

packages/types/src/middleware.ts

+    /**
+     * An object that contains options to pass to the underlying http client.
+     */
+    httpOptions?: HttpOptions;


Fix: HttpOptions -> HttpType

…nted as a class. Model added as HandlerArgs

chrisradek · 2017-11-02T05:42:07Z

Updates:

CoreHandler is now a separate package, and accepts an httpHandler when instantiated.
HttpHandlers are now classes, instead of functions. They can be instantiated with HttpOptions, such as timeouts. The idea is that httpOptions will be shared across all commands against a client context.
model is now part of HandlerArguments. If a HttpHandler/CoreHandler can be set once on a client context, but used for multiple commands, then they need to have the Operation Model available after they are created. It made sense to me to put this alongside input and request, let me know what you think.

Example usage:

    const httpHandler = new NodeHttpHandler(/* HttpOptions */{
        socketTimeout: 20 //ms
    });
    const restParser = new RestParser(
        new JsonParser(
            fromBase64
        ),
        streamCollector,
        toUtf8,
        fromBase64
    );
    const coreHandler = new CoreHandler(
        httpHandler,
        restParser
    );
    /** Fake calling coreHandler at end of Middleware Stack */
    let response = await coreHandler.handle({
        request: signedRequest,
        input: {},
        model: ListFunctions
    });

jeskew · 2017-11-02T17:40:20Z

I'm not crazy about including the model in the arguments passed from handler to handler, but I'm also not sure I like how complex the alternatives would need to be. Those might include:

Requiring the model to be passed into the constructor. Whoever creates the context object must also use the model included there to create the core handler. This seems easy but could make it hard to support customer-supplied core handlers (which are extremely useful for tests that involve exercising complex middleware stacks).
Defining a CoreHandlerConstructor interface that looks roughly like:

interface CoreHandlerCtor<Input, Output, Stream> {
    new (context: HandlerExecutionContext): Handler<Input, Output, Stream>;
}

The middleware stack's resolve method could take that instead of Handler and then instantiate it with the context. (It could also be a property on the stack to make resolve easier to call; this is what PHP and Ruby do, I believe.) You would pass in the additional constructor arguments by subclassing, subclassing anonymously, or with bind.

2 Feels a bit more sound but maybe too complicated. It would also require instantiating more core handlers, which may not be as performant as simply including more properties in the HandlerArguments object.

…o CoreHandler constructor

…ng change

…g port to address issue

chrisradek · 2017-11-16T07:17:14Z

sigh I made code changes so that the model is no longer part of the handler args. This should be ready for a final review. Unfortunately, my node-http-handler tests (relies on a mock server) are failing with a networking error, and it only happens in code build (works using the same and other versions of node.js on local machine). I've tried the suggested tips online...so I'm approaching the point where I may just rip out using an actual server.

...I'll see how it looks in the morning.

jeskew

LGTM, pending the CodeBuild-only test failures getting sorted out.

I had a couple nits and questions, but none of them were blocking.

jeskew · 2017-11-16T05:38:20Z

packages/core-handler/src/index.ts

+        InputType extends object,
+        OutputType extends MetadataBearer,
+        StreamType = Uint8Array
+    > implements Handler<InputType, OutputType, StreamType> {


Very nit: extra indentation

jeskew · 2017-11-16T06:17:45Z

packages/node-http-handler/src/node-http-handler.ts

+            // if the request was already aborted, prevent doing extra work
+            if (abortSignal && abortSignal.aborted) {
+                const abortError = new Error('Request aborted');
+                abortError.name = 'AbortError';


I've seen this pair of lines in a few places. Might make sense to have an AbortError constructor that automatically sets the name to 'AbortError'

Yeah, there was an error in JS implementations when subclassing an error. Basically, instanceof Error checks returned false on subclasses of errors (I think there was a work-around if you weren't using es2015 classes).

That said, I can at the very least create refactor this into a function.

jeskew · 2017-11-16T06:23:05Z

packages/node-http-handler/src/node-http-handler.ts

+                // pipe automatically handles end
+                return;
+            } else if (request.body) {
+                req.write(request.body);


I think you can just use req.end(request.body) here to eliminate both the need for the separate call to .end on line 93 and the early return on line 89.

Good point, that'll work!

jeskew · 2017-11-16T06:34:58Z

packages/node-http-handler/src/set-connection-timeout.ts

+        return;
+    }
+
+    request.on('socket', function(this: ClientRequest, socket: Socket) {


I'm not sure how I feel about calling a function parameter this. On my first reading of the callback, I thought you were able to call this.abort() due to some undocumented binding on node's part.

Same as comment below: this isn't a function parameter, it's used by Typescript to get the function's context.

Side-note: VSCode recognizes this and uses a different color for this than other parameters 😄

jeskew · 2017-11-16T06:36:43Z

packages/stream-collector-node/src/index.ts

+import {StreamCollector} from '@aws/types';
+import {Collector} from './collector';
+
+export const streamCollector: StreamCollector<Readable> = function streamCollector(


Why not just export function streamCollector?

I did it this way so I could define the type StreamCollector<Readable>, and enforce that the function conforms to the StreamCollector interface. I had tried writing it as just a function and use the as type at the end, but that didn't work.

If you know a way to accomplish this, I'd be all for changing this to a function.

jeskew · 2017-11-16T06:43:36Z

packages/stream-collector-node/src/index.ts

+export const streamCollector: StreamCollector<Readable> = function streamCollector(
+    stream
+): Promise<Uint8Array> {
+    return new Promise((resolve, reject) => {


This shouldn't occur when collecting an HTTP stream, but you would want to throw if the provided stream has an encoding set.

So I looked into this some more. There isn't a public interface for getting the encoding of the source stream. (stream._readableState.encoding)

That said, I'm not sure we need to worry about it. Strings will automatically get converted to buffers as data is being piped. The user would have to set the encoding on the source stream to be different than what the actual source is, which at that point they're essentially shooting themselves in the foot.

Fair point. Objection withdrawn.

jeskew · 2017-11-16T06:48:00Z

packages/stream-collector-node/src/index.ts

+        });
+        collector.on('error', reject);
+        collector.on('finish', function(this: Collector) {
+            const bytes = new Uint8Array(Buffer.concat(this.bufferedBytes));


I'm still a bit thrown off by the use of this.

Just to record the explanation somewhere:
TS 2.0 introduced a way to specify what this should be inside of a function:
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0.html

See Specifying the type of this for functions

jeskew · 2017-11-16T20:18:18Z

packages/types/src/http.ts

+ * Represents the http options that can be passed to a browser http client.
+ */
+export interface BrowserHttpOptions extends HttpOptions {
+    requestTimeout?: number;


Since we have to manually timeout requests when using fetch, couldn't this property just be part of HttpOptions?

Yes... I didn't want to also write this functionality for Node.js unless it was specifically asked for, which is why I only put it on the BrowserHttpOptions for now.

If you feel strongly that we should have request timeouts (separate from connection/socket-read timeouts) in node.js as well, I can add that.

jeskew · 2017-11-16T20:59:45Z

packages/fetch-http-handler/src/fetch-http-handler.ts

+                    } else {
+                        return response.arrayBuffer()
+                            .then(buffer => {
+                                httpResponse.body = buffer;


Could this be changed to httpResponse.body = new Uint8Array(buffer)? This won't copy the buffer and allows us to declare the StreamType of browser requests to be ReadableStream|Uint8Array, which is easier for end users to disambiguate than ReadableStream|ArrayBuffer due to the availability of ArrayBuffer.isView.

* Adds stream collectors * Adds node-http-handler and fetch-http-handler * Adds core-handler

lock · 2019-09-26T20:05:09Z

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.

chrisradek added 7 commits October 15, 2017 18:46

Adds stream collectors

7eb8e7f

Adds initial http support

9ada29e

Adds timeouts and tests to http-node package

18c4167

Updates timeout errors

088e869

Merge branch 'master' into http

467856a

Merge remote-tracking branch 'upstream/master' into http

71c6c56

Updates http handlers to use class-based middleware pattern

090ba8d

chrisradek changed the title ~~[WIP] Http Handlers~~ Http Handlers Oct 31, 2017

chrisradek assigned jeskew and AllanZhengYP Oct 31, 2017

chrisradek commented Oct 31, 2017

View reviewed changes

chrisradek added 2 commits November 1, 2017 22:18

Splits up CoreHandler into separate package. HttpHandlers now represe…

b481e29

…nted as a class. Model added as HandlerArgs

Merge remote-tracking branch 'upstream/master' into http

f2f51b2

chrisradek added 2 commits November 1, 2017 23:04

Fixes test issues that occur when using a clean environment

0427827

Fix FetchHttpHandler to work on older versions of node

2cbc35e

chrisradek added 8 commits November 9, 2017 13:43

Merge remote-tracking branch 'upstream/master' into http

1912bf3

Remove model from HandlerArguments and pass HandlerExecutionContext t…

2d83506

…o CoreHandler constructor

Removes model from middleware stack tests

5d787ea

Cast IncommingHttpHeader to string to work-around node typings breaki…

e19642b

…ng change

CodeBuild test fails due to networking error, passes locally. Changin…

e2430c8

…g port to address issue

Battling against CodeBuild

fa660d0

strengthen mock server port acquisition

aec635e

Rage against the machine

41f3fc9

jeskew approved these changes Nov 16, 2017

View reviewed changes

Switch tests to use ipv4

db0393a

jeskew reviewed Nov 16, 2017

View reviewed changes

chrisradek added 2 commits November 16, 2017 16:48

Address PR feedback

0df11f3

Correcting for developer error

13786ea

chrisradek merged commit b798cec into aws:master Nov 17, 2017

hyandell unassigned jeskew and AllanZhengYP Jul 20, 2018

trivikr referenced this pull request in trivikr/aws-sdk-js-v3 Dec 10, 2018

Http Handlers (#57)

53941c3

* Adds stream collectors * Adds node-http-handler and fetch-http-handler * Adds core-handler

lock bot locked as resolved and limited conversation to collaborators Sep 26, 2019

Http Handlers #57

Http Handlers #57

Uh oh!

Conversation

chrisradek commented Oct 23, 2017

Uh oh!

chrisradek commented Oct 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisradek commented Nov 2, 2017

Uh oh!

jeskew commented Nov 2, 2017

Uh oh!

chrisradek commented Nov 16, 2017

Uh oh!

jeskew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lock bot commented Sep 26, 2019

Uh oh!

Uh oh!