Skip to content

Conversation

@wemeetagain
Copy link
Contributor

This allows :content to be a stream.

If :content is a stream and :content-length is a specified header,
set :content-length and copy that many bytes.

If :content is a stream and :content-length is not specified,
set :transfer-encoding to chunked, and copy the entire stream.

The :content-type in both cases is defaulted to application/octet-stream

The only worrying thing is that since reading from a stream can only be done once, any attempts at auto-reconnect will have unexpected behavior, as the stream will possibly be emptied.

Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove content-type here?

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 appears to be a list of headers which is not allowed to be set manually.

The content-type is automatically set in all cases but when a stream :content is set. L459

I found it useful and necessary to manually set the content-type in this situation, since the type cannot be gleaned from the :content itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm, if you look at the definition of write-header* it will write the user supplied header if specified, or the header passed in if not. So this loop prevents writing the header twice if a header that is always supplied is specified by the user. With you change, if the user supplies a content-type header and a non-stream content, then the Content-Type header will be written twice.

Probably, you should instead have a (write-header* :content-type "application/octet-stream") if the content is chunked. "application/octet-stream" should also probably be the default when a byte array is passed as the content, rather than "text/plain". But that is tangential.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

d'oh, gotcha. 😊 I'll do that.

I'll leave the byte array content-type for another PR.

@tmccombs
Copy link
Contributor

tmccombs commented Nov 1, 2015

One improvement would be to use a non-chunked content if the user supplies the Content-Length header, and then only copy Content-Length bytes.

@wemeetagain
Copy link
Contributor Author

@tmccombs Agreed, that would be better. I've have added a commit to do so.

Bear in mind that there will be an issue if the contents is read and then,
for whatever reason, the auto-restart is triggered.
If :content-length is set, and :content is a stream, only copy
that many bytes, otherwise, set transfer-encoding to chunked, and copy
the entirety of the stream.
If :content is a stream and :content-length is a specified header,
set :content-length and copy that many bytes.

If :content is a stream and :content-length is not specified,
set :transfer-encoding to chunked, and copy the entire stream.

The :content-type in both cases is defaulted to "application/octet-stream"
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.

2 participants