Skip to content

Commit

Permalink
fix(dev): break implicit rerouting loop (withastro#10737)
Browse files Browse the repository at this point in the history
* fix(dev): infinite implicit rerouting

* test adapter

* changeset
  • Loading branch information
lilnasy authored and PeterDraex committed Apr 23, 2024
1 parent 4dd37e2 commit b15ae18
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 50 deletions.
5 changes: 5 additions & 0 deletions .changeset/sharp-elephants-clap.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"astro": patch
---

Fixes a regression where constructing and returning 404 responses from a middleware resulted in the dev server getting stuck in a loop.
2 changes: 1 addition & 1 deletion packages/astro/src/vite-plugin-astro-server/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ export async function handleRoute({
}
if (response.status === 404 && response.headers.get(REROUTE_DIRECTIVE_HEADER) !== 'no') {
const fourOhFourRoute = await matchRoute('/404', manifestData, pipeline);
if (options)
if (options && options.route !== fourOhFourRoute?.route)
return handleRoute({
...options,
matchedRoute: fourOhFourRoute,
Expand Down
96 changes: 47 additions & 49 deletions packages/astro/test/custom-404-implicit-rerouting.test.js
Original file line number Diff line number Diff line change
@@ -1,69 +1,67 @@
import assert from 'node:assert/strict';
import { after, before, describe, it } from 'node:test';
import { loadFixture } from './test-utils.js';
import testAdapter from './test-adapter.js';

for (const caseNumber of [1, 2, 3, 4]) {
for (const caseNumber of [1, 2, 3, 4, 5]) {
describe(`Custom 404 with implicit rerouting - Case #${caseNumber}`, () => {
/** @type Awaited<ReturnType<typeof loadFixture>> */
/** @type {import('./test-utils.js').Fixture} */
let fixture;
/** @type Awaited<ReturnType<typeof fixture['startDevServer']>> */
let devServer;

before(async () => {
fixture = await loadFixture({
output: 'server',
root: `./fixtures/custom-404-loop-case-${caseNumber}/`,
site: 'http://example.com',
adapter: testAdapter()
});

devServer = await fixture.startDevServer();
});

// sanity check
it.skip(
'dev server handles normal requests',
{
todo: 'To re-enabled after we understand why this fails when the test suite is run in parallel',
},
async () => {
const resPromise = fixture.fetch('/');
const result = await withTimeout(resPromise, 1000);
assert.notEqual(result, timeout);
assert.equal(result.status, 200);
}
);
describe("dev server", () => {
/** @type {import('./test-utils.js').DevServer} */
let devServer;

it.skip(
'dev server stays responsive',
{
todo: 'To re-enabled after we understand why this fails when the test suite is run in parallel',
},
async () => {
const resPromise = fixture.fetch('/alvsibdlvjks');
const result = await withTimeout(resPromise, 1000);
assert.notEqual(result, timeout);
assert.equal(result.status, 404);
}
);
before(async () => {
await fixture.build()
devServer = await fixture.startDevServer();
});

after(async () => {
await devServer.stop();
// sanity check
it('dev server handles normal requests', { timeout: 1000 }, async () => {
const response = await fixture.fetch('/');
assert.equal(response.status, 200);
});

// IMPORTANT: never skip
it('dev server stays responsive', { timeout: 1000 }, async () => {
const response = await fixture.fetch('/alvsibdlvjks');
assert.equal(response.status, 404);
});

after(async () => {
await devServer.stop();
});
});
});
}

describe("prod server", () => {
/** @type {import('./test-utils.js').App} */
let app;

/***** UTILITY FUNCTIONS *****/

const timeout = Symbol('timeout');

/** @template Res */
function withTimeout(
/** @type Promise<Res> */
responsePromise,
/** @type number */
timeLimit
) {
/** @type Promise<typeof timeout> */
const timeoutPromise = new Promise((resolve) => setTimeout(() => resolve(timeout), timeLimit));
before(async () => {
app = await fixture.loadTestAdapterApp();
});

return Promise.race([responsePromise, timeoutPromise]);
// sanity check
it('prod server handles normal requests', { timeout: 1000 }, async () => {
const response = await app.render(new Request('https://example.com/'));
assert.equal(response.status, 200);
});

// IMPORTANT: never skip
it('prod server stays responsive', { timeout: 1000 }, async () => {
const response = await app.render(new Request('https://example.com/alvsibdlvjks'));
assert.equal(response.status, 404);
});
});
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<p>all good here... or is it?</p>

0 comments on commit b15ae18

Please sign in to comment.