History

scope requests

Rhys Sullivan committed 6 months ago

Commit b60caa51 parent d062f092 files · +67 −7Browse files

Unified · Split

2 files
  1. apps/cloud/src/api.request-scope.node.test.ts +49 −2
  2. apps/cloud/src/api/request-scoped.ts +18 −5
apps/cloud/src/api.request-scope.node.test.ts+49−2
@@ -34,10 +34,18 @@ class Counter extends Context.Service< · 33 unmodified lines
3434 { readonly id: number }
3535 >()("test/Counter") {}
3636
37−const makeCounterLive = (counts: { acquires: number; releases: number }) =>
37+const makeCounterLive = (
38+ counts: { acquires: number; releases: number },
39+ acquireDelayMs = 0,
40+) =>
3841 Layer.effect(Counter)(
3942 Effect.acquireRelease(
40− Effect.sync(() => {
43+ Effect.gen(function* () {
44+ // Yield to the event loop inside acquire to force concurrent
45+ // request fibers to overlap on the shared boot MemoMap.
46+ if (acquireDelayMs > 0) {
47+ yield* Effect.sleep(`${acquireDelayMs} millis`);
48+ }
4149 counts.acquires += 1;
4250 return { id: counts.acquires };
4351 }),
@@ -114,6 +122,45 @@ describe("HttpRouter.toWebHandler request scoping", () => { · 70 unmodified lines
114122 expect(counts.acquires).toBe(2);
115123 expect(counts.releases).toBe(2);
116124 });
125+
126+ // Concurrent regression: Cloudflare Workers serves multiple in-flight
127+ // requests from the same isolate. `Layer.build(layer)` (used by
128+ // `requestScopedMiddleware`) inherits the boot-level `CurrentMemoMap`
129+ // installed by `HttpRouter.toWebHandler`, so two requests that race
130+ // through the middleware before either's scope closes BOTH reuse the
131+ // first request's memoized layer build — sharing one postgres.js socket
132+ // across two request handlers, which the runtime forbids:
133+ // "Cannot perform I/O on behalf of a different request"
134+ //
135+ // The fix must give each request a fresh MemoMap so memoization is
136+ // request-local. Without it, this test acquires only once (and would
137+ // crash in prod on the second concurrent request's I/O).
138+ it("requestScopedMiddleware does NOT share a build across concurrent requests", async () => {
139+ const counts = { acquires: 0, releases: 0 };
140+ // 5ms async sleep inside acquire forces the two request fibers to
141+ // overlap on the layer build, the same shape as Cloudflare Workers
142+ // serving multiple in-flight requests from one isolate.
143+ const App = Routes.pipe(
144+ Layer.provide(requestScopedMiddleware(makeCounterLive(counts, 5)).layer),
145+ Layer.provideMerge(HttpServer.layerServices),
146+ );
147+ const handler = HttpRouter.toWebHandler(App, { disableLogger: true })
148+ .handler;
149+
150+ const [a, b] = await Promise.all([
151+ handler(new Request("http://test.local/")),
152+ handler(new Request("http://test.local/")),
153+ ]);
154+
155+ const aBody = await a.json();
156+ const bBody = await b.json();
157+ // Two concurrent requests must see two distinct acquired counters.
158+ // Otherwise both fibers share one postgres socket -> Cloudflare
159+ // Workers I/O isolation crash in prod.
160+ expect(new Set([aBody.id, bBody.id])).toEqual(new Set([1, 2]));
161+ expect(counts.acquires).toBe(2);
162+ expect(counts.releases).toBe(2);
163+ });
117164 });
118165
119166 // ---------------------------------------------------------------------------
apps/cloud/src/api/request-scoped.ts+18−5
@@ -12,12 +12,21 @@ · 11 unmodified lines
1212 // `Layer.provideMerge` and (despite the name) `HttpRouter.provideRequest`
1313 // both build the inner layer at construction time. The only primitive
1414 // that actually rebuilds per request is a router middleware whose
15−// per-request handler calls `Layer.build(layer)` inside `Effect.scoped`,
16−// so `acquireRelease` fires per request and finalizers run when the
17−// request fiber's scope closes.
15+// per-request handler builds the layer with a *fresh* `MemoMap` and a
16+// per-request scope, so `acquireRelease` fires per request and finalizers
17+// run when the request fiber's scope closes.
18+//
19+// The fresh `MemoMap` matters: `Layer.build` would otherwise inherit
20+// `CurrentMemoMap` from the boot context (`HttpRouter.toWebHandler`
21+// installs one when it builds the app layer). Cloudflare Workers serves
22+// concurrent requests from the same isolate, and the boot MemoMap is
23+// shared across those request fibers — so two in-flight requests would
24+// both reuse the first one's memoized layer build (one postgres socket
25+// shared across two request handlers, which the runtime forbids). A
26+// per-request MemoMap scopes memoization to a single request fiber.
1827 //
1928 // See `apps/cloud/src/api.request-scope.node.test.ts` for the regression
20−// coverage that pins this rule down.
29+// coverage that pins this rule down (sequential AND concurrent cases).
2130 // ---------------------------------------------------------------------------
2231
2332 import { Effect, Layer } from "effect";
@@ -40,7 +49,11 @@ export const requestScopedMiddleware = <A>(layer: Layer.Layer<A>) => · 16 unmodified lines
4049 HttpRouter.middleware<{ provides: A }>()((httpEffect) =>
4150 Effect.scoped(
4251 Effect.gen(function* () {
43− const services = yield* Layer.build(layer);
52+ // Fresh MemoMap per request — see file-level note for why we
53+ // must NOT inherit `CurrentMemoMap` from the boot context.
54+ const memoMap = yield* Layer.makeMemoMap;
55+ const scope = yield* Effect.scope;
56+ const services = yield* Layer.buildWithMemoMap(layer, memoMap, scope);
4457 return yield* Effect.provideContext(httpEffect, services);
4558 }),
4659 ),

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t