Skip to content
This repository has been archived by the owner on Mar 1, 2023. It is now read-only.

SapiStreamEmitter that does not consider Content-Range #46

Open
Stadly opened this issue Sep 13, 2021 · 4 comments
Open

SapiStreamEmitter that does not consider Content-Range #46

Stadly opened this issue Sep 13, 2021 · 4 comments

Comments

@Stadly
Copy link

Stadly commented Sep 13, 2021

Is your feature request related to a problem? Please describe.

SapiStreamEmitter::emit considers the header Content-Range and emits only the relevant range when only a single range is requested and the range unit is bytes. This seems very nice and convenient at first glance.

If the application wants to support multi-range requests or range units other than bytes, however, populating the response with the correct content range(s) must be done by the application, and SapiStreamEmitter will emit the entire provided response body.

This seems very asymmetrical to me. Who should actually be in charge of ensuring that the response body is correct? The one who populates the response body or the one who emits it?

My application would have to populate the final response body in the case of multi-range or non-byte unit, but populate the entire file into the response body in the case of single byte range, and then let SapiStreamEmitter handle the range-stuff.

Describe the solution you'd like

I would like the option to let SapiStreamEmitter just output the whole response body that I have provided, regardless of the Content-Range range header.

I actually don't think the Content-Range functionality has anything to do in the emitter, so I would prefer to just remove it. But such a change in functionality might warrant a new major version. The behavior could also be set in a constructor argument. Or there could be a separate class.

If the Content-Range functionality has anything to do in the emitter, why isn't it also a part of the SapiEmitter. In my opinion, exchanging the one for the other should not lead to different output.

Describe alternatives you've considered

Teachability, Documentation, Adoption, Migration Strategy

@github-actions
Copy link
Contributor

Awesome! Thank you for taking the time to create your first issue! Please review the guidelines

@Stadly
Copy link
Author

Stadly commented Sep 14, 2021

To better illustrate the issue, here are a few examples:

Example of request and response for single byte range, where the entire file contents are abcdefghijklmnopqrstuvwxyz.

Request

GET /url/to/file HTTP/1.1
Range: bytes=5-15

Response

HTTP/1.1 206 Partial Content
Accept-Ranges: bytes
Content-Length: 11
Content-Range: bytes 5-15/26

fghijklmnop

When emitting this response, SapiStreamEmitter will only emit klmnop as the body, since the first 5 bytes are omitted. SapiEmitter will emit the whole fghijklmnop.

Example of request and response for multiple byte ranges (the 4th byte, bytes 11 to 21, the last 5 bytes, bytes 19 and to the end), where the entire file contents are abcdefghijklmnopqrstuvwxyz.

Request

GET /url/to/file HTTP/1.1
Range: bytes=bytes=3-3, 10-20, -5, 18-

Response

HTTP/1.1 206 Partial Content
Accept-Ranges: bytes
Content-Length: 227
Content-Type: multipart/byteranges; boundary=BOUNDARY

--BOUNDARY
Content-Range: bytes 3-3/26

d
--BOUNDARY
Content-Range: bytes 10-20/26

klmnopqrstu
--BOUNDARY
Content-Range: bytes 21-25/26

vwxyz
--BOUNDARY
Content-Range: bytes 18-25/26

stuvwxyz
--BOUNDARY--

When emitting this response, both SapiStreamEmitter and SapiEmitter will emit the whole response body, since there is no Content-Range header (it is embedded into the body for multi-range requests).

@prisis
Copy link
Member

prisis commented Jan 20, 2022

Thanks for you time to descript the issue, and i'm really sorry that i'm responding only right now.

I will try to come with a good solution, im happy if you have on too :)

@Stadly
Copy link
Author

Stadly commented Jan 21, 2022

After more consideration, I think the best solution is to fix the memory issue in SapiEmitter.

Is there a reason why SapiEmitter converts the whole response body to a string and emits it in a single go instead of emitting it as smaller chunks? With the current implementation of SapiEmitter, the entire response body must be written to memory, which does not work for large responses.

When the memory issue is fixed, SapiEmitter can be used when the application takes care of populating the response body correctly, and SapiStreamEmitter can be used to provide a simple support for single-range requests for bytes.

Alternative solutions are:

  1. Modify SapiStreamEmitter to allow ignoring the content range.
  2. Create a separate emitter. (It will function exactly like SapiEmitter, just without the memory issue, so that's the use?)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants