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

Update serve-static.ts #18

Closed
wants to merge 7 commits into from
Closed

Update serve-static.ts #18

wants to merge 7 commits into from

Conversation

Hoodgail
Copy link
Contributor

Mistakes from the original code.

const content = readFileSync(path)

This will load the whole file into memory.
If you attempt to serve an MP4 that is 1GB in size, 1GB will be loaded into memory. As a result, if you attempt to load that file 10 times and your server or computer only has 10GB of RAM, your memory will run out and your client will fail.
Use streams and range headers as a solution.
See Range headers for more information.


if (c.finalized) {
await next()
}

The request will go on to the next handler if it is finished, but the code at the bottom will continue to execute as if the request was never concluded in the first place.


const mimeType = getMimeType(path)

This issue has nothing to do with this repo, but it was missing a LOT of mimetypes, like MP4 videos, gltf, glb 3d files etc, i made a separate pull request to its repo at honojs/hono#827

@Hoodgail
Copy link
Contributor Author

Works as expected

image

I just spot an other issue, if a path has spaces or some other special letters in them, most browsers will encode it as a URI component, so we have to decode it.
@Hoodgail
Copy link
Contributor Author

I Found one issue with my pull request

image
image

206 partial content is not supported by the cache API, i'll fix it later, i forgot that the 206 status code is only sent once the client adds the the range header on the requests after the initial one, 200 is sent to the initial response

I'll fix when i'm available later.

The status code should only be 206 when range headers are preasent, else, it should be 200
@yusukebe
Copy link
Member

Hi @Hoodgail !

Thank you for the PR. Please do three things:

  1. Prettify source code. npx prettier src/serve-static.ts --write
  2. Fix the type error as shown below.
  3. Write proper tests.

SS

@Hoodgail
Copy link
Contributor Author

@yusukebe I made the first two fixes, but the test functions are already valid

@yusukebe
Copy link
Member

Thanks. But, we have to add the following tests:

  it('Should return correct headers and data with range headers', async () => {
    let res = await request(server).get('/static/plain.txt').set('range', '0-9')
    expect(res.status).toBe(206)
    expect(res.headers['content-type']).toBe('text/plain; charset=utf-8')
    expect(res.headers['content-length']).toBe('10')
    expect(res.headers['content-range']).toBe('bytes 0-9/17')
    expect(res.text.length).toBe(10)
    expect(res.text).toBe('This is pl')

    res = await request(server).get('/static/plain.txt').set('range', '10-16')
    expect(res.status).toBe(206)
    expect(res.headers['content-type']).toBe('text/plain; charset=utf-8')
    expect(res.headers['content-length']).toBe('7')
    expect(res.headers['content-range']).toBe('bytes 10-16/17')
    expect(res.text.length).toBe(7)
    expect(res.text).toBe('ain.txt')
  })

And we also have to consider the case of exceptions.

await request(server).get('/static/plain.txt').set('range', '0-17')

Then it will be aborted.

@Hoodgail
Copy link
Contributor Author

image

@yusukebe
Copy link
Member

@Hoodgail

Thanks. Then how about this case?

it.only('Should not be aborted', async () => {
  await request(server).get('/static/plain.txt').set('range', '0-17')
})

This will fail with "aborted".

SS

I think we have to handle the error in some way.

@mindplay-dk
Copy link
Contributor

mindplay-dk commented Mar 4, 2023

This is a good next step towards production ready static file server middleware. 👍

We need to eventually consider cache-headers (If-Modified-Since, ETag, etc.) in order to have predictable performance and reliable interop with e.g. cache proxy servers and CDNs etc. - there's a very complete implementation for Koa here that could be referenced.

I think, ideally, we should consider something like this proposal in order to avoid writing and maintaining complex static file-server logic for every platform - the proposition there is about interop between frameworks, but the same problem appears to apply to middleware even within the framework, and might could be addressed by something like this.

Don't let me distract you too much from just getting a working file-server going for Node though - that's still a useful step, and once that's proven, maybe we could start to think about extracting/abstracting the platform differences to achieve a production-ready static file service across platforms. 🤔

@Hoodgail
Copy link
Contributor Author

@yusukebe I don't understand what you mean, shoulnt it be aborted after the all of the content is received? Unless there is a keep-alive header

Connect Handler type for the middleware function
@tangye1234
Copy link
Contributor

tangye1234 commented Mar 31, 2023

Temperially I use a code snippet like this, I think serve-static is not production ready, event if it is almost getting close.
I only need to send a single file, so this code is not a replacement, while it may be a reference I can contribute :)

import { createReadStream, Stats } from 'fs'
import { stat } from 'fs/promises'
import type { Context } from 'hono'
import { getMimeType } from 'hono/utils/mime'
// Here I use Readable.toWeb, which is supported only in nodejs 18+, which can be replaced by many other means
import { Readable } from 'stream'

export function fileToResponse(
  c: Context<any, any, any>,
  path: string,
  type?: string
) {
  return stat(path)
    .then(stat => stream(c, path, stat, type), () => c.notFound())
    .catch((e: any) =>
      c.json({
        success: false,
        message: String(e?.message || 'Unknown error'),
      }, 500)
    )
}

function stream(
  c: Context<any, any, any>,
  path: string,
  stat: Stats,
  type?: string
) {
  if (!stat.isFile()) {
    return c.notFound()
  }

  const size = stat.size
  type = type || getMimeType(path) || 'application/octet-stream'

  const headers = {
    'Content-Type': type,
    'Content-Length': String(size),
  } as {
    'Content-Type': string
    'Content-Length': string
    'Content-Range'?: string
  }

  if (size === 0) {
    return c.body(null, 200, headers)
  }

  const range = c.req.header('range') || ''

  if (!range) {
    return c.body(Readable.toWeb(createReadStream(path)), 200, headers)
  }

  const m = range.match(/^(?<units>[^=]+)=(?<start>\d*)-(?<end>\d*)$/)

  const { units, start, end } = m?.groups || {}
  if (!m || !units) {
    return c.body(null, 400)
  }

  if (m[0].length !== range.length) {
    // not support multipart range request
    return c.body(null, 400)
  }

  if (units !== 'bytes') {
    return c.body(null, 400)
  }

  if (!start && !end) {
    // FIXME supported?
    return c.body(Readable.toWeb(createReadStream(path)), 200, headers)
  }

  if (!start) {
    const len = Math.min(Number(end), size)
    if (len === 0) {
      return c.body(null, 400)
    }

    if (len === size) {
      return c.body(Readable.toWeb(createReadStream(path)), 200, headers)
    }

    const s = size - len
    const e = size - 1

    headers['Content-Length'] = String(len)
    headers['Content-Range'] = `bytes ${s}-${e}/${size}`

    return c.body(
      Readable.toWeb(createReadStream(path, { start: s, end: e })),
      206,
      headers
    )
  }

  const e = Math.min(Number(end || size - 1), size)
  const s = Math.min(Number(start), e)
  const len = e - s + 1
  if (len === 0) {
    return c.body(null, 400)
  }

  if (len === size) {
    return c.body(Readable.toWeb(createReadStream(path)), 200, headers)
  }

  headers['Content-Length'] = String(len)
  headers['Content-Range'] = `bytes ${s}-${e}/${size}`

  return c.body(
    Readable.toWeb(createReadStream(path, { start: s, end: e })),
    206,
    headers
  )
}

@that-ambuj
Copy link
Contributor

that-ambuj commented Jun 22, 2023

Hi @Hoodgail, I was the last person that worked on the serve-static middleware and I had built it from reference using Deno's serve-static middleware from the main repository. I see that my implementation had some issues but I'm also glad that I could learn from my mistakes. Since the current implementation was taken from the main repo, I think that you should apply these improvements there too.

@schonert
Copy link

Any news on this?

@yusukebe
Copy link
Member

@schonert

Nothing. I'll be addressing this issue now.

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

Hi @Hoodgail

I apologize for the delay.

I've commented on the edge case. If you agree with my points, could you please revise your code and resolve the conflicts? Then, I can approve it.

if (range) {
const parts = range.replace(/bytes=/, '').split('-')
start = parseInt(parts[0], 10)
end = parts[1] ? parseInt(parts[1], 10) : end
Copy link
Member

Choose a reason for hiding this comment

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

We have to add the following code if the client range header value exceeds the data size:

if (stat.size < end - start + 1) {
  end = stat.size - 1
}

And should add the tests:

it('Should return correct headers and data if client range exceeds the data size', async () => {
  const res = await request(server).get('/static/plain.txt').set('range', '0-20')
  expect(res.status).toBe(206)
  expect(res.headers['content-type']).toBe('text/plain; charset=utf-8')
  expect(res.headers['content-length']).toBe('17')
  expect(res.headers['content-range']).toBe('bytes 0-16/17')
  expect(res.text.length).toBe(17)
  expect(res.text).toBe('This is plain.txt')
})

@mindplay-dk
Copy link
Contributor

The implementation posted by @tangye1234 looks really clean and readable, with good separation of concerns - each function "does one thing and does it well". Should be very easy to unit test as well.

It would be great to have both the stream and fileToResponse functions not only as implementation details in a static file server, but exported - as these would be really helpful in specialized middleware besides static file-servers, e.g. caches or proxies to other servers or cloud file storage, image servers, and so on.

@tangye1234 with regards to your // FIXME supported?, yes, responding to a range request without a range header is permitted - servers are not required to support range, and the range header in the response indicates to the client that header was accepted and that the response is in fact partial; clients do not (must not) assume they will get a range response.

@yusukebe
Copy link
Member

The PR #63 based on this PR is merge now. Thanks!

@yusukebe yusukebe closed this Jul 15, 2023
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.

6 participants