Repository navigation
fix(tools): address follow-up review comments from #9533 - #9553
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Bun shim configuration in bin/proxyquire-bun-shim.cjs to support shorter environment variable aliases, switches generational cache management to use enableRequireShim, and refactors the fetch interception logic to activate under multiple shim flags. Additionally, --proxyquire-shim is added to the Spanner observability tests. The review feedback highlights two important issues in the fetch shim: a potential infinite recursion/stack overflow if globalThis.fetch is reassigned to the shimmed fetch, and a memory leak caused by not removing the 'abort' event listener from init.signal once a request completes.
| init = {...init, body: Readable.toWeb(stream)}; | ||
| } | ||
| try { | ||
| const res = await globalThis.fetch(url, init); |
There was a problem hiding this comment.
If globalThis.fetch is ever reassigned to globalThis.__googleCloudBunFetch (for example, by another shim or plugin to globally apply the fetch interception), calling globalThis.fetch(url, init) here will cause infinite recursion and a stack overflow, because globalThis.fetch === origFetch will be false, bypassing the interception block and falling back to calling itself.
To prevent this, we should fall back to origFetch if globalThis.fetch is globalThis.__googleCloudBunFetch.
const res = await (globalThis.fetch === globalThis.__googleCloudBunFetch ? origFetch : globalThis.fetch)(url, init);
There was a problem hiding this comment.
Fixed in 741191f — updated the HTTP/HTTPS transport check and the fallback call to use origFetch if globalThis.fetch === globalThis.__googleCloudBunFetch, preventing infinite recursion.
| init.signal.addEventListener('abort', () => { | ||
| req.destroy( | ||
| Object.assign(new Error('The user aborted a request.'), { | ||
| name: 'AbortError', | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The 'abort' event listener is added to init.signal but is never removed when the request completes successfully or fails. If init.signal is a long-lived AbortSignal (which is common when reused across multiple requests), this will cause a memory leak and may trigger a MaxListenersExceededWarning in Node.js/Bun.
We should define a named listener and remove it when the request finishes or errors using the request's 'close' event.
const abortHandler = () => {
req.destroy(
Object.assign(new Error('The user aborted a request.'), {
name: 'AbortError',
}),
);
};
init.signal.addEventListener('abort', abortHandler);
req.on('close', () => {
init.signal.removeEventListener('abort', abortHandler);
});
There was a problem hiding this comment.
Fixed in 741191f — added a named abortHandler with {once: true} and removed it on the request's 'close' event to avoid leaking listeners on long-lived AbortSignals.
…rt listener cleanup
2 participants
Description
Addresses all 7 follow-up review comments from #9533 in the Bun test shim and package test scripts.
Impact
Improves environment variable consistency, avoids unnecessary
Proxywrapping ofrequire.cachefor packages that only use--proxyquire-shim, preventsTypeError: globalThis.__googleCloudBunFetch is not a functionwhen--bun-plugin-shimor--gaxios-shimis enabled without--fetch-shim, enables standalone execution of Spanner'sobservability-testunder Bun, and removes the temporary full-suite CI trigger comment fromci/run_conditional_tests.sh.Changes
BUN_ENABLE_<NAME>_SHIM === 'true'andBUN_<NAME>_SHIM === 'true'inbin/proxyquire-bun-shim.cjsso the short environment variable names also work when Mocha is invoked directly withoutbin/run-test.cjs.createCacheGenerationProxyonModule._cacheandrequire.cacheunderenableRequireShiminstead ofenableProxyquireShim, since onlymockeryinhandwritten/storagereassignsModule._cache = ....AbortSignal.timeoutcomment inbin/proxyquire-bun-shim.cjsto referencecore/packages/gaxiosinstead ofcore/packages/gcp-metadata.globalThis.__googleCloudBunFetchwheneverenableFetchShim || enableBunPluginShim || enableGaxiosShimis true (keeping theteeny-requestModule._extensions['.js']hook gated underenableFetchShim), and indent the body ofglobalThis.__googleCloudBunFetchby 2 spaces.--proxyquire-shimto theobservability-testscript inhandwritten/spanner/package.json.ci/run_conditional_tests.sh.Testing
BUN_<NAME>_SHIM=trueenvironment variables activate their respective shims when loadingbin/proxyquire-bun-shim.cjsdirectly.globalThis.__googleCloudBunFetchis defined when eitherBUN_GAXIOS_SHIM=trueorBUN_PLUGIN_SHIM=trueis enabled withoutBUN_FETCH_SHIM=true.Module._cache/require.cachegenerational cache proxying is enabled underBUN_REQUIRE_SHIM=true.Alternatives
mockeryfromhandwritten/storage/test/resumable-upload.tsinstead of gatingcreateCacheGenerationunderenableRequireShim, but gatingcreateCacheGenerationunderenableRequireShimavoids modifying storage test dependencies while still eliminating therequire.cacheProxyfor the other 13--proxyquire-shimpackages.--bun-plugin-shimand--gaxios-shimdependent on--fetch-shimbeing passed simultaneously and would cause every subsequent PR to run all 5 CI unit test shards due to the trigger comment inci/run_conditional_tests.sh.Fixes b/571038362