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

Support overriding Content-Length header #5

Closed
syogel opened this issue Aug 3, 2021 · 4 comments · Fixed by #705
Closed

Support overriding Content-Length header #5

syogel opened this issue Aug 3, 2021 · 4 comments · Fixed by #705
Labels
Rust SDK Parity Tracking label for features available in the C@E Rust SDK

Comments

@syogel
Copy link

syogel commented Aug 3, 2021

Client set up a new JS C@E service and everything is working correctly with the exception that our content-length header is not being returned. Turns out in practice this doesn’t matter a ton because clients are looking for the content-range which is being returned correctly. But we would definitely want this fixed before completely switching over to C@E in production.

@tschneidereit

@williamoverton
Copy link

Example URL: https://pipe-stream.edgecompute.app/stream?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE2MjQzMDkyMDAwMDAsImV4cCI6MTYyNDM5NTYwMDAwMCwiZGF0YSI6eyJ1IjpbImh0dHBzOi8vY2h1bWxleS5iYXJzdG9vbHNwb3J0cy5jb20vYmFyYmEvc3BsaXRGaWxlLXNlZ21lbnQtMDAwMC5tcDMiLCJodHRwczovL2NodW1sZXkuYmFyc3Rvb2xzcG9ydHMuY29tL2JhcmJhL3NwbGl0RmlsZS1zZWdtZW50LTAwMDEubXAzIiwiaHR0cHM6Ly9jaHVtbGV5LmJhcnN0b29sc3BvcnRzLmNvbS9iYXJiYS9zcGxpdEZpbGUtc2VnbWVudC0wMDAyLm1wMyJdLCJjIjoiYXVkaW8vbXBlZyJ9fQ.daGc_ubCDmBZw_2JfepyFd7N_kdX0OnirQwF59VfJDQ

code:

return new Response(destStream, {
    status: 206,
    headers: {
      'accept-ranges': 'bytes',
      'content-type': contentType,
      'content-length': contentLength.toString(),
      'content-range': `bytes ${contentRange}/${fullContentLength}`
    }
  })

Things of note:

  • Using streams
  • Sending a 206 (Partial Content)
  • Including a content-range

@williamoverton
Copy link

Appears to be a limitation in xqd that you can never have a content-length on streams since you can not set the header and xqd has no way of knowing the body size at the time of sending headers.

Feels like we need to implement responsesToAppend from AS -> JS. (and possibly poke xqd to see why we cant set content-length on streams)

@tschneidereit
Copy link
Contributor

One potential workaround is to ensure that the stream has been closed by the time the Response is sent. That would mean only creating the actual Response object once the stream has been fully written to.

As all solutions that work around the limitation in the underlying platform — that content can't set the content-length header explictly — this is somewhat bad for performance in cases where we otherwise could just pipe an upstream response body through, though.

@tschneidereit tschneidereit changed the title JS - content-length issue Support overriding Content-Length header Apr 20, 2022
@tschneidereit tschneidereit added the Rust SDK Parity Tracking label for features available in the C@E Rust SDK label Apr 20, 2022
@tschneidereit
Copy link
Contributor

C@E now supports overriding the Content-Length header using the ManuallyFromHeaders framing-headers mode.

The right way to expose this is probably as a property on the Response constructor's init parameter, e.g. overrideContentLength: true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust SDK Parity Tracking label for features available in the C@E Rust SDK
Projects
None yet
3 participants