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

[PoC] feat: introduce Partial Content Middleware #3516

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yusukebe
Copy link
Member

@yusukebe yusukebe commented Oct 14, 2024

This PR will introduce a new middleware, "Partial Content Middleware".

It's based on #3461 by @exoego . That PR means the Serve Static Middleware supports 206 Partial Content. But this PR can support all contents not only by Serve Static Middleware:

import { partialContent } from 'hono/partial-content'

app.use(partialContent())

app.get('/hello.jpg', (c) => {
  return c.body(body, {
    headers: {
      'Content-Length': body.length.toString(),
      'Content-Type': 'image/jpeg'
    },
  })
})

Then, the /hello.jpg can support 206 Partial Content. It will be enabled when the Response has a Content-Legnth header. The partial contents will be returned based on those values. So, if you want to use it with Serve Satatic, it should return the correct content length with Content-Length --- but the current Serve Static Middleware does not emit content length. So, to do it, we have to modify it to get the file size using a method such as Bun.file() and set it as content length.

This PR will be co-authored by @exoego

We have to discuss this feature.

Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 97.02970% with 3 lines in your changes missing coverage. Please review.

Project coverage is 94.32%. Comparing base (9986b47) to head (79ebfef).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/middleware/partial-content/index.ts 97.02% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3516      +/-   ##
==========================================
- Coverage   95.58%   94.32%   -1.26%     
==========================================
  Files         155      158       +3     
  Lines        9357     9589     +232     
  Branches     2749     2786      +37     
==========================================
+ Hits         8944     9045     +101     
- Misses        413      544     +131     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yusukebe yusukebe changed the title [WIP] feat: introduce Partial Content Middleware [PoC] feat: introduce Partial Content Middleware Oct 14, 2024
@yusukebe yusukebe marked this pull request as ready for review October 15, 2024 10:25
@yusukebe
Copy link
Member Author

Hey @exoego @usualoma @nakasyou and others

What do you think about this?

@exoego
Copy link
Contributor

exoego commented Oct 15, 2024

I like this design since it is much simpler and supports various adapters and middlewares.

One small concern is performance. This design assumes an underlying stream supports range access via Stream.getReader/subarray. But if the underlying Stream reads bytes from the beginning eagerly, range access is not efficient.
So it is better to document such note for users.

On the other hand, #3461 requires middleware to provide range access.

@nakasyou
Copy link
Contributor

Most of JS runtimes's file system API provides seek API. e.g.:

using file = await Deno.open('./image.jpg', { read: true })

await file.seek(6) // Move the file pointer 6 bytes forward
// ...

If the middleware or serveStatic can access those APIs, performance problem might be resolved. In web standards, there is FileSystemWritableFileStream in File System Access API. I think there is also a solution to use this.

@usualoma
Copy link
Member

Hi @yusukebe!

What are the real-world problems we need to solve?

If the case of returning “206 Partial Content” is almost always serve-static, and if it is assumed to be used in a production environment, it seems better to be implemented in the serve-static feature rather than being made independent as middleware. There are no users who want to use serve-static but don't want to support Range headers, so I think it would be more convenient if Range headers were supported automatically when using serve-static.
And as mentioned in the review comments of the other two people, I think it would be better to use a lower-level API to improve performance.

Is there a use case other than serve-static? If there are use cases, I think it would be best to make them into separate middleware like this pull request and then optimize serve-static by adding a separate abstraction layer.

@exoego
Copy link
Contributor

exoego commented Oct 16, 2024

For Hono users on AWS or GCP (not Cloudflare), serving content from object storage (like AWS S3 or GCP Cloud Storage) will be an use-case of Partial Content middleware, without Server Static middleware.

@usualoma
Copy link
Member

Hi @exoego, Thanks for the reply.

For Hono users on AWS or GCP (not Cloudflare), serving content from object storage (like AWS S3 or GCP Cloud Storage) will be an use-case of Partial Content middleware, without Server Static middleware.

I simply don't know much about this, and I'd like to ask for some guidance, but if the app is acting as a proxy for AWS S3 or GCP Cloud Storage, I think it would be better to pass the Range header straight through to the storage and return the response as is, but is that not possible?

If you're caching the data in your app, that's one thing, but if you're not caching it, I think it would be a bit of a pain to “fetch all the data from AWS S3 or GCP Cloud, put it in memory, and then return a portion of it by specifying the Renge header”.

@yusukebe
Copy link
Member Author

That's an interesting topic. Quick response!

Also, if you use Cloudflare R2, which is object storage, you have to create Workers with Hono to distribute assets (though there is another way to distribute it without Workers):

import { Hono } from 'hono';

type Env = {
  Bindings: {
    BUCKET: R2Bucket
  }
}

const app = new Hono<Env>()

app.get('/', async (c) => {
  const file = await c.env.BUCKET.get('file-name')
  return new Response(file?.body)
})

@exoego
Copy link
Contributor

exoego commented Oct 16, 2024

It looks like AWS S3 supports partial content natively. One needs to specifie range options.
So Partial Content middlware appears not needed for S3.
https://docs.aws.amazon.com/AmazonS3/latest/userguide/download-objects.html#download-objects-parts
https://stackoverflow.com/questions/36436057/s3-how-to-do-a-partial-read-seek-without-downloading-the-complete-file

So please forget AWS S3 (I don't know GCP though)

@exoego
Copy link
Contributor

exoego commented Oct 16, 2024

app.get('/', async (c) => {
  const file = await c.env.BUCKET.get('file-name')
  return new Response(file?.body)
})

In this example of Cloudflare R2, the underlying file is fully-read, which is not ideal for Partial Content.
Big use-case of partial content is video seeking, so full-read should be avoided.

@yusukebe
Copy link
Member Author

I've investigated. For Cloudflare R2, c.env.BUCKET.get can range access to the object. So, we may not need this middleware for it!

@yusukebe
Copy link
Member Author

Indeed, there may not be a use case.

BUT, I am personally concerned about Serve Static getting fat. That needs to be re-designed and discussed.

@yusukebe
Copy link
Member Author

@exoego @nakasyou @usualoma

Anyway, thank you for your comments!

I'll make this PR as a draft and prepare to discuss about Serve Static and others.

Now, we are implementing runtime-specific matters like file system access and WebSockets in adapters in hono package. This approach is good but it is getting fat to support many runtimes.

I think it's time to re-think the scope of the hono package and re-designed how to treat non-standard things.

Related issues and PRs:

#3483
#3461
#3307

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.

4 participants