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

fix: add cancellation to AsyncIterable correctly and leak #4887

Merged
merged 6 commits into from
Dec 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .changeset/brown-vans-search.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@graphql-tools/executor-http': patch
'@graphql-tools/utils': patch
---

Fix leak on Node 14 and add cancellation to async iterables correctly
5 changes: 5 additions & 0 deletions .changeset/cuddly-wasps-listen.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@graphql-tools/delegate': patch
---

Fix handling variables
8 changes: 5 additions & 3 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ jobs:
- name: Build Packages
run: yarn build
- name: Test
run: yarn jest --no-watchman --ci browser
env:
TEST_BROWSER: true
uses: nick-fields/retry@v2
with:
timeout_minutes: 10
max_attempts: 5
command: TEST_BROWSER=true yarn jest --no-watchman --ci browser
4 changes: 2 additions & 2 deletions packages/delegate/src/finalizeGatewayRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
SelectionSetNode,
TypeInfo,
TypeNameMetaFieldDef,
valueFromAST,
valueFromASTUntyped,
VariableDefinitionNode,
versionInfo as graphqlVersionInfo,
visit,
Expand Down Expand Up @@ -225,7 +225,7 @@ function updateArguments(
let value: any;
const existingValueNode = argumentNodeMap[argName]?.value;
if (existingValueNode != null) {
value = valueFromAST(existingValueNode, argType, variableValues);
value = valueFromASTUntyped(existingValueNode, variableValues);
}
if (value == null) {
value = serializeInputValue(argType, newArgs[argName]);
Expand Down
9 changes: 7 additions & 2 deletions packages/executors/http/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ import {
SyncExecutor,
} from '@graphql-tools/utils';
import { GraphQLResolveInfo, print } from 'graphql';
import { cancelNeeded } from './addCancelToResponseStream.js';
import { isLiveQueryOperationDefinitionNode } from './isLiveQueryOperationDefinitionNode.js';
import { prepareGETUrl } from './prepareGETUrl.js';
import { ValueOrPromise } from 'value-or-promise';
import { createFormDataFromVariables } from './createFormDataFromVariables.js';
import { handleEventStreamResponse } from './handleEventStreamResponse.js';
import { handleMultipartMixedResponse } from './handleMultipartMixedResponse.js';
import { fetch as defaultFetch, AbortController } from '@whatwg-node/fetch';
import { cancelNeeded } from './addCancelToResponseStream.js';

export type SyncFetchFn = (url: string, init?: RequestInit, context?: any, info?: GraphQLResolveInfo) => SyncResponse;
export type SyncResponse = Omit<Response, 'json' | 'text'> & {
Expand Down Expand Up @@ -78,7 +78,7 @@ export function buildHTTPExecutor(
export function buildHTTPExecutor(options?: HTTPExecutorOptions): Executor<any, HTTPExecutorOptions> {
const executor = (request: ExecutionRequest<any, any, any, HTTPExecutorOptions>) => {
const fetchFn = request.extensions?.fetch ?? options?.fetch ?? defaultFetch;
const controller = cancelNeeded() ? new AbortController() : undefined;
let controller: AbortController | undefined;
let method = request.extensions?.method || options?.method || 'POST';

const operationAst = getOperationASTFromRequest(request);
Expand Down Expand Up @@ -113,13 +113,18 @@ export function buildHTTPExecutor(options?: HTTPExecutorOptions): Executor<any,

let timeoutId: any;
if (options?.timeout) {
controller = new AbortController();
timeoutId = setTimeout(() => {
if (!controller?.signal.aborted) {
controller?.abort();
}
}, options.timeout);
}

if (!controller && cancelNeeded()) {
controller = new AbortController();
}

return new ValueOrPromise(() => {
switch (method) {
case 'GET': {
Expand Down
13 changes: 5 additions & 8 deletions packages/loaders/url/tests/url-loader-browser.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,14 +259,11 @@ describe('[url-loader] webpack bundle compat', () => {
document as any
);

expect(results).toEqual([
{ data: { countdown: [] } },
{ data: { countdown: [3] } },
{ data: { countdown: [3, 2] } },
{ data: { countdown: [3, 2, 1] } },
{ data: { countdown: [3, 2, 1, 0] } },
{ data: { countdown: [3, 2, 1, 0] } },
]);
expect(results[0]).toEqual({ data: { countdown: [] } });
expect(results[1]).toEqual({ data: { countdown: [3] } });
expect(results[2]).toEqual({ data: { countdown: [3, 2] } });
expect(results[3]).toEqual({ data: { countdown: [3, 2, 1] } });
expect(results[4]).toEqual({ data: { countdown: [3, 2, 1, 0] } });
});

it('handles SSE subscription operations', async () => {
Expand Down
2 changes: 2 additions & 0 deletions packages/loaders/url/tests/yoga-compat.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { useEngine } from '@envelop/core';
import { useDeferStream } from '@graphql-yoga/plugin-defer-stream';

describe('Yoga Compatibility', () => {
jest.setTimeout(10000);
const loader = new UrlLoader();
let httpServer: http.Server;
const liveQueryStore = new InMemoryLiveQueryStore();
Expand Down Expand Up @@ -151,6 +152,7 @@ describe('Yoga Compatibility', () => {
if (httpServer !== undefined) {
await new Promise<void>(resolve => httpServer.close(() => resolve()));
}
await sleep(1000);
});

it('should handle defer', async () => {
Expand Down
8 changes: 2 additions & 6 deletions packages/utils/src/withCancel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,11 @@ export function getAsyncIterableWithCancel<T, TAsyncIterable extends AsyncIterab
const existingPropValue = Reflect.get(asyncIterable, prop, receiver);
if (Symbol.asyncIterator === prop) {
return function asyncIteratorFactory() {
const asyncIterator: AsyncIterator<T> = Reflect.apply(
existingPropValue[Symbol.asyncIterator],
asyncIterable,
[]
);
const asyncIterator: AsyncIterator<T> = Reflect.apply(existingPropValue as any, asyncIterable as any, []);
return getAsyncIteratorWithCancel(asyncIterator, onCancel);
};
} else if (typeof existingPropValue === 'function') {
return proxyMethodFactory(asyncIterable, existingPropValue[Symbol.asyncIterator]);
return proxyMethodFactory<any, any>(asyncIterable, existingPropValue);
}
return existingPropValue;
},
Expand Down
18 changes: 18 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1670,6 +1670,24 @@
dependencies:
giscus "^1.2.3"

"@graphql-tools/[email protected]":
version "0.0.9"
resolved "https://registry.yarnpkg.com/@graphql-tools/executor/-/executor-0.0.9.tgz#f55f8cbe12d7989b0ff58cca9a041fbdde3dbd40"
integrity sha512-qLhQWXTxTS6gbL9INAQa4FJIqTd2tccnbs4HswOx35KnyLaLtREuQ8uTfU+5qMrRIBhuzpGdkP2ssqxLyOJ5rA==
dependencies:
"@graphql-tools/utils" "9.1.1"
"@graphql-typed-document-node/core" "3.1.1"
"@repeaterjs/repeater" "3.0.4"
tslib "^2.4.0"
value-or-promise "1.0.11"

"@graphql-tools/[email protected]":
version "9.1.1"
resolved "https://registry.yarnpkg.com/@graphql-tools/utils/-/utils-9.1.1.tgz#b47ea8f0d18c038c5c1c429e72caa5c25039fbab"
integrity sha512-DXKLIEDbihK24fktR2hwp/BNIVwULIHaSTNTNhXS+19vgT50eX9wndx1bPxGwHnVBOONcwjXy0roQac49vdt/w==
dependencies:
tslib "^2.4.0"

"@graphql-tools/utils@^8.5.2", "@graphql-tools/utils@^8.8.0":
version "8.13.1"
resolved "https://registry.yarnpkg.com/@graphql-tools/utils/-/utils-8.13.1.tgz#b247607e400365c2cd87ff54654d4ad25a7ac491"
Expand Down