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

Questions about streaming and Content-Length #1086

Closed
david-perez opened this issue Feb 9, 2022 · 6 comments · Fixed by #1152
Closed

Questions about streaming and Content-Length #1086

david-perez opened this issue Feb 9, 2022 · 6 comments · Fixed by #1152

Comments

@david-perez
Copy link
Contributor

What should Content-Length be set to for streaming operations in the different AWS protocols?

The restJson1 spec does not prescribe anything. However, the protocol test suite contains this test:

{
    id: "RestJsonStreamingTraitsRequireLengthWithBlob",
    documentation: "Serializes a blob in the HTTP payload with a required length",
    protocol: restJson1,
    code: 200,
    body: "blobby blob blob",
    bodyMediaType: "application/octet-stream",
    headers: {
        "X-Foo": "Foo",
        "Content-Type": "application/octet-stream"
    },
    requireHeaders: [
        "Content-Length"
    ],
    params: {
        foo: "Foo",
        blob: "blobby blob blob"
    }
}

Under what circumstances should streaming operations include the Content-Length header?

Here it's clear it should be set to 16 (which by the way, the test does not assert, it only asserts the presence of the header, why?), but for an arbitrary stream the client / server might not know in advance the size of the payload. I therefore think a better name for this test would be something like RestJsonStreamingTraitsRequireLengthWithBlobOfKnownSize (unless of course the header should always be set in restJson1, in which case I'd like to know what value it should be set to if the stream's size is not known in advance).

@mtdowling
Copy link
Member

This particular test targets a blob that has the @requiresLength trait: https://github.com/awslabs/smithy/blob/main/smithy-aws-protocol-tests/model/restJson1/streaming.smithy#L186-L196. This means that blob is streaming and not something you'd fit into memory, but the size must be known before streaming. Content-Length should be set when possible, and in this case it's possible because you know the length.

I agree the name could be misleading. Would you be able to send a PR to change the name? Also feel free to make the content-length check more specific rather than a presence check if you like.

@david-perez
Copy link
Contributor Author

Ah, I completely missed requiresLength, that makes sense. The test name is not so bad then :)

Four follow-up questions. With requiresLength, if the user sets a streaming blob whose exact size cannot be known in advance:

  1. Should a client fail to send a request?
  2. Should a server reject a request?
  3. Should a client fail to deserialize a response?
  4. Should a server prevent the service from sending the response?

The spec is worded with MUST,

In an HTTP-based protocol, for instance, this trait indicates that the Content-Length header MUST be sent prior to a client or server sending the payload of a message.

so I'd expect the answers to all 4 questions would be "Yes", but it'd be nice if the spec explicitly called out these cases.

I also think "Yes" to all 4 questions might be too harsh of a requirement. I always associate the usefulness of streaming with being able to stream from file descriptors, where it's very hard in most cases to know the size of the stream in advance without buffering the entire thing into memory. I can only foresee requiresLength being honored as a MUST in the case where you've written an async data generator and you know the size of the final data that you'll stream, which is a very narrow use.

@gosar
Copy link
Contributor

gosar commented Feb 14, 2022

My understanding is that for the more general case where length won't be known in advance and the server is ok to support that case, the model just won't have requiresLength.

@gosar
Copy link
Contributor

gosar commented Feb 14, 2022

Re: your follow ups - Yeah I think 1 and 2 is Yes. 4 also is Yes - why allow a service to not send Content-Length when it has modeled it as requiresLength. 3 I'm a bit unsure - I'm debating if it is too harsh for a client to reject a response if it could still process it anyways, though I think it would be better to fail since the consumer of the client code may care and expect to know the size.

@gosar
Copy link
Contributor

gosar commented Mar 3, 2022

I think the remaining follow up on this issue is to looking at making requiresLength applicable to input shapes only.

@gosar gosar closed this as completed Mar 14, 2022
@gosar gosar reopened this Mar 14, 2022
@mtdowling
Copy link
Member

Should a client fail to send a request?

I think so. I've changed my mind on this over the years.

If the client tries to resolve this, it's potentially making big assumptions that the stream is repeatable or can be buffered to disk. Callers trying to send an unbounded stream will need to do this work themselves, and they're the only ones in the position to make the decision as to how they'll provide the length.

Should a server reject a request?

Yes

Should a client fail to deserialize a response?
Should a server prevent the service from sending the response

No to both. This should only be honored on requests. Sending a PR now.

mtdowling added a commit that referenced this issue Mar 23, 2022
We have no use case of a server needing to be constrained by
requiresLength. We can always expand this trait later if needed since
it's an annotation trait.

Closes #1086
mtdowling added a commit that referenced this issue Mar 23, 2022
We have no use case of a server needing to be constrained by
requiresLength. We can always expand this trait later if needed since
it's an annotation trait.

Closes #1086
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 a pull request may close this issue.

3 participants