Skip to content

Commit

Permalink
fix: add cancellation to AsyncIterable correctly and leak (#4887)
Browse files Browse the repository at this point in the history
* fix: add cancellation to AsyncIterable correctly and leak

* Go

* Go

* Go

* Hola

* Go
  • Loading branch information
ardatan authored Dec 7, 2022
1 parent 90011f0 commit 904fe77
Show file tree
Hide file tree
Showing 9 changed files with 52 additions and 21 deletions.
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

0 comments on commit 904fe77

Please sign in to comment.