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

Corrupted file served if file is changed during serving. #155

Open
wdanilo opened this issue Jan 20, 2023 · 4 comments
Open

Corrupted file served if file is changed during serving. #155

wdanilo opened this issue Jan 20, 2023 · 4 comments

Comments

@wdanilo
Copy link

wdanilo commented Jan 20, 2023

Hi, I've got such a code for my server:

import path from 'node:path'
import url from 'node:url'
import portfinder from 'portfinder'
import connect from 'connect'
import { WebSocketServer } from 'ws'
import serveStatic from 'serve-static'
import logger from 'morgan'

export const DEFAULT_PORT = 8080

let dirname = path.dirname(url.fileURLToPath(import.meta.url))
// Path of a file that needs to be injected into the bundle for live-reload to work.
export const LIVE_RELOAD_LISTENER_PATH = path.join(dirname, 'live-reload.js')

export async function start({ root, assets, port }) {
    assets = assets ?? path.join(root, 'assets')

    const freePort = await portfinder.getPortPromise({ port: port ?? DEFAULT_PORT })

    const setHeaders = (res) => {
        res.setHeader('Cache-Control', 'no-store')
    }

    const app = connect()
        .use(logger('dev', { skip: (req, res) => res.statusCode < 400 }))
        .use(serveStatic(root, {setHeaders}))
        .use('/assets', serveStatic(assets, {setHeaders}))

    const server = app.listen(freePort)
    const wsServer = new WebSocketServer({ server, clientTracking: true, path: '/live-reload' })

    var serverUrl = `http://localhost:${freePort}`
    console.log('Serving %s', serverUrl)

    return {
        port: freePort,
        reload() {
            wsServer.clients.forEach(sock => sock.send('reload'))
        },
    }
}

And we have observed a very interesting behavior. Namely, when it is serving our WASM file (100MB+), if the WASM is being re-build just when serving starts, then it is served to the browser corrupted. What's interesting is that after it is rebuilt, browser refreshes dont help - the same corrupted file is being served. What helps is restarting the above server to serve non-corrupted file. Does anyone have any idea why this happens and if this can be prevented somehow? I'm OK with serving it in a corrupted state for the first time (during rebuild), but I want the file to be served correctly after page reload.

@dougwilson
Copy link
Contributor

Hi @wdanilo that is strange indeed! This module doesn't perform any type of internal caching of anything, from the file content to any other metadata. Each request for a given file is looked up in the file system and read a-new every time. The reading is done using the fs.createReadStream Node.js API (https://nodejs.org/docs/latest-v17.x/api/fs.html#fscreatereadstreampath-options) and the data from that stream (created new for every request) is directed passed to the Node.js HTTP response object (https://nodejs.org/docs/latest-v17.x/api/http.html#class-httpserverresponse).

I could try and help debug the cause if I can get the exact code and steps to reproduce the issue, though.

@wdanilo
Copy link
Author

wdanilo commented Jan 20, 2023

@dougwilson thanks so much for fast reply, so detailed answer, and that you are willing to help us. I appreciate it a lot! Unfortunately, this issue is not easily reproducible on our end, as it happens sometimes, when certain conditions are met (we are unsure what are these conditions yet, we have only observed that it happens when the WASM file is being regenerated during serving). We will try to configure the code to reproduce it on every build and we will get back to you :)

@autopulated
Copy link

The problem is likely that you are modifying the file in-place on the filesystem when regenerating it. Instead, generate the new version under a different filename, and then move it to replace the old one. This way a single read stream of the file will either get the complete old or complete new version of file.

@keithchew
Copy link

keithchew commented Oct 16, 2023

Hi @wdanilo @dougwilson

Under the hood, serve-static uses the send package, and there is a race condition in that package, ie if the file contents changes after the fs.stat call but while streaming is still going to the client, the contents received by the client will be corrupted. Reference to the issue here:

pillarjs/send#122

The relevant code snippet:

SendStream.prototype.sendFile = function sendFile (path) {
  ...
  fs.stat(path, function onstat (err, stat) {
    ...
    ***** corruption if file contents changed here *****
    self.send(path, stat)
  })

I am also experiencing this issue, where Express serves a PNG back to the browser. If the PNG content gets overwritten while Chrome is getting the image, it will be appear corrupted on the webpage.

Note: Even if you did a "write to temp file" and atomically rename to destination file (ie what @autopulated suggested), it is still susceptible to the race condition above.

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

No branches or pull requests

4 participants