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.

Sign up for GitHub

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

perf: middleware to short-circuit on 304 #15586

Merged
merged 5 commits into from
Jan 19, 2024
Merged

perf: middleware to short-circuit on 304 #15586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f24ab2d
perf: middleware to short-circuit on 304
patak-dev Jan 12, 2024
1c4ec51
fix: move after cors middleware
patak-dev Jan 12, 2024
7f189df
fix: mixed etag for direct CSS that is also imported
patak-dev Jan 12, 2024
c5820eb
chore: merge main
patak-dev Jan 18, 2024
e9a6011
fix: adding a public file with the same path as a module
patak-dev Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions packages/vite/src/node/server/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ import { createWebSocketServer } from './ws'
import { baseMiddleware } from './middlewares/base'
import { proxyMiddleware } from './middlewares/proxy'
import { htmlFallbackMiddleware } from './middlewares/htmlFallback'
import { transformMiddleware } from './middlewares/transform'
import {
cachedTransformMiddleware,
transformMiddleware,
} from './middlewares/transform'
import {
createDevHtmlTransformFn,
indexHtmlMiddleware,
Expand Down Expand Up @@ -681,7 +684,17 @@ export async function _createServer(

if (publicDir && publicFiles) {
if (file.startsWith(publicDir)) {
publicFiles[isUnlink ? 'delete' : 'add'](file.slice(publicDir.length))
const path = file.slice(publicDir.length)
publicFiles[isUnlink ? 'delete' : 'add'](path)
if (!isUnlink) {
const moduleWithSamePath = await moduleGraph.getModuleByUrl(path)
const etag = moduleWithSamePath?.transformResult?.etag
if (etag) {
// The public file should win on the next request over a module with the
// same path. Prevent the transform etag fast path from serving the module
moduleGraph.etagToModuleMap.delete(etag)
}
}
}
}
await handleFileAddUnlink(file, server, isUnlink)
Expand Down Expand Up @@ -751,6 +764,8 @@ export async function _createServer(
middlewares.use(corsMiddleware(typeof cors === 'boolean' ? {} : cors))
}

middlewares.use(cachedTransformMiddleware(server))

// proxy
const { proxy } = serverConfig
if (proxy) {
Expand Down
43 changes: 31 additions & 12 deletions packages/vite/src/node/server/middlewares/transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,37 @@ const debugCache = createDebugger('vite:cache')

const knownIgnoreList = new Set(['/', '/favicon.ico'])

/**
* A middleware that short-circuits the middleware chain to serve cached transformed modules
*/
export function cachedTransformMiddleware(
server: ViteDevServer,
): Connect.NextHandleFunction {
// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
return function viteCachedTransformMiddleware(req, res, next) {
// check if we can return 304 early
const ifNoneMatch = req.headers['if-none-match']
if (ifNoneMatch) {
const moduleByEtag = server.moduleGraph.getModuleByEtag(ifNoneMatch)
Copy link
Member

Choose a reason for hiding this comment

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

If a file is added in public directory, I think we need to delete a entry from server.moduleGraph.etagToModuleMap.
For example, if /src/foo.ts existed and later on /public/src/foo.ts is added, then /public/src/foo.ts should be returned instead of the transpileed /src/foo.ts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

More context for others. We are serving public files using sirv, and I see that the way it computes its etag is:

if (isEtag) headers['ETag'] = `W/"${stats.size}-${stats.mtime.getTime()}"`;

There shouldn't be any overlap between public etags and processed etags.

But as this two URLs are the same, the browser may request the public file using the etag of processed module because of the same issue discussed here.

But if the source file existed but was removed, the etag is no longer going to be there. The only case where this is an issue is when there are both a public and a source file at the same time (and the public file was added later). If we find the module by id and remove its etag, then the browser may still request the url with the wrong etag but it should still pick up the public file.

I think it should be fixed by e9a6011

if (moduleByEtag?.transformResult?.etag === ifNoneMatch) {
// For direct CSS requests, if the same CSS file is imported in a module,
// the browser sends the request for the direct CSS request with the etag
// from the imported CSS module. We ignore the etag in this case.
const mixedEtag =
!req.headers.accept?.includes('text/css') &&
isDirectRequest(moduleByEtag.url)
Comment on lines +60 to +65
Copy link
Member

Choose a reason for hiding this comment

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

Trying to understand this part. Do you mean that the browser would send the same etag for text/css requests and *.css imports? That sounds strange. Is it a bug that also happens before?

I think it would still be nice to make the request work instead of bailing here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this guard, this test was failing. The issue is that in that test we are both adding global.css as a link in the html and then importing it. I tried to find a reason why we do so and I couldn't. But I think we should leave it as is because it actually helped to spot this bug.

I think our current system for .css isn't ideal. What we do is adding a ?direct flag when we detect that global.css was requested as a link. But that ?direct flag is internal, for the browser, it sees two global.css requests with the same URL. We return different etags for each. When the browser requests the imported global.css module the second time, it uses the etag of the linked one. I thought it could be a bug in chrome but I tested in Firefox and Safari and in all the browsers is the same.

This playground was still working because we did a getModuleByUrl before this PR, and it was done after adding ?direct when it was a link. If the etag was mixed for imported css modules, then it will not match with the etag of the module and a regular 200 will be issued instead of a 304. So this PR is doing the same for this case, I don't know how we could avoid bailing out here. I think it is ok as the setup is an edge case.

An alternative would be to switch from ?direct for linked CSS to using ?import for imported CSS modules instead like we do with other assets (rewriting during import analysis). I think this would actually be cleaner. If we do so, then a bare .css request will always correspond to a link request, and a .css?import will always be a module and these URLs are seen by the browser (and not like ?direct that we add internally) so the requests are for different URLs. But I don't know if we can do this change given that it may have a big effect on the ecosystem, and it is hard to justify it when it is only an issue if the same .css is linked and imported at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

Ah thanks. Sounds like a weird browser bug, but maybe for good reasons. Yeah in that case if it's rare, then I think we can simply bail then. Maybe we could add a debug message to detect this kind of requests if it later turns out to be more common than expected.

The ?import idea does make more sense to me, but yeah maybe we can leave that until we need to make a bigger change.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @sapphi-red, this may be interesting to you as I think you mentioned we could remove ?import at one point, no? If we do so, we may hit this same etag issues for that assets.

Copy link
Member

Choose a reason for hiding this comment

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

I guess Vary header (Vary: Accept) is what we should use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh... this looks great, thanks @sapphi-red! I tried it out and it seems it fixes safari but I still see the same issue in Chrome and Firefox. So maybe there are browsers bugs here? I think we should keep the guard for now and aim to use Vary header in the future (for when we'll try to remove ?import)

Copy link
Member

Choose a reason for hiding this comment

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

It seems I misunderstood the Vary header. Vary header doesn't affect conditional requests.
Also browsers seems to only use a single E-Tag in If-None-Match header. I guess there isn't a way to tell browsers to send different If-None-Match header for a same URL and a different header.

if (!mixedEtag) {
debugCache?.(`[304] ${prettifyUrl(req.url!, server.config.root)}`)
res.statusCode = 304
return res.end()
}
}
}

next()
}
}

export function transformMiddleware(
server: ViteDevServer,
): Connect.NextHandleFunction {
Expand Down Expand Up @@ -155,18 +186,6 @@ export function transformMiddleware(
url = injectQuery(url, 'direct')
}

// check if we can return 304 early
const ifNoneMatch = req.headers['if-none-match']
if (
ifNoneMatch &&
(await server.moduleGraph.getModuleByUrl(url, false))?.transformResult
?.etag === ifNoneMatch
) {
debugCache?.(`[304] ${prettifyUrl(url, server.config.root)}`)
res.statusCode = 304
return res.end()
}

// resolve, load and transform using the plugin container
const result = await transformRequest(url, server, {
html: req.headers.accept?.includes('text/html'),
Expand Down
25 changes: 25 additions & 0 deletions packages/vite/src/node/server/moduleGraph.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export type ResolvedUrl = [
export class ModuleGraph {
urlToModuleMap = new Map<string, ModuleNode>()
idToModuleMap = new Map<string, ModuleNode>()
etagToModuleMap = new Map<string, ModuleNode>()
// a single file may corresponds to multiple modules with different queries
fileToModulesMap = new Map<string, Set<ModuleNode>>()
safeModulesPath = new Set<string>()
Expand Down Expand Up @@ -192,6 +193,9 @@ export class ModuleGraph {

// Don't invalidate mod.info and mod.meta, as they are part of the processing pipeline
// Invalidating the transform result is enough to ensure this module is re-processed next time it is requested
const etag = mod.transformResult?.etag
if (etag) this.etagToModuleMap.delete(etag)

mod.transformResult = null
mod.ssrTransformResult = null
mod.ssrModule = null
Expand Down Expand Up @@ -419,6 +423,27 @@ export class ModuleGraph {
return this._resolveUrl(url, ssr)
}

updateModuleTransformResult(
mod: ModuleNode,
result: TransformResult | null,
ssr: boolean,
): void {
if (ssr) {
mod.ssrTransformResult = result
} else {
const prevEtag = mod.transformResult?.etag
if (prevEtag) this.etagToModuleMap.delete(prevEtag)

mod.transformResult = result

if (result?.etag) this.etagToModuleMap.set(result.etag, mod)
}
}

getModuleByEtag(etag: string): ModuleNode | undefined {
return this.etagToModuleMap.get(etag)
}

/**
* @internal
*/
Expand Down
12 changes: 4 additions & 8 deletions packages/vite/src/node/server/transformRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,10 +361,8 @@ async function loadAndTransform(

// Only cache the result if the module wasn't invalidated while it was
// being processed, so it is re-processed next time if it is stale
if (timestamp > mod.lastInvalidationTimestamp) {
if (ssr) mod.ssrTransformResult = result
else mod.transformResult = result
}
if (timestamp > mod.lastInvalidationTimestamp)
moduleGraph.updateModuleTransformResult(mod, result, ssr)

return result
}
Expand Down Expand Up @@ -465,10 +463,8 @@ async function handleModuleSoftInvalidation(

// Only cache the result if the module wasn't invalidated while it was
// being processed, so it is re-processed next time if it is stale
if (timestamp > mod.lastInvalidationTimestamp) {
if (ssr) mod.ssrTransformResult = result
else mod.transformResult = result
}
if (timestamp > mod.lastInvalidationTimestamp)
server.moduleGraph.updateModuleTransformResult(mod, result, ssr)

return result
}