diff --git a/packages/node-core/CHANGELOG.md b/packages/node-core/CHANGELOG.md index 2f95f969af..9675d1b3b3 100644 --- a/packages/node-core/CHANGELOG.md +++ b/packages/node-core/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed - Dictionary v1 queries kept every parsed query document in memory: each batch's query text is new, and both `graphql-tag`'s cache and Apollo Client's `QueryManager.transformCache` retained it, so a long sync's memory grew without bound. Queries are now posted as plain GraphQL over HTTP and nothing is parsed or cached (#3048) +- Dictionary v1 queries threw a `TypeError` on a `null` body, `{data: null}` or `errors` that isn't an array, and their errors dropped the response body, so the `distinct` fallback missed non-standard error responses. The error now keeps the body and includes it in its message (#3051) +- A timed-out dictionary v1 query kept running in the background with its connection and response body. It is now aborted a second after the query times out (#3051) ### Changed - `DictionaryV1` sends queries with a protected `query()` method instead of an Apollo `client`, and `@apollo/client` is no longer a dependency (#3048) diff --git a/packages/node-core/src/indexer/dictionary/v1/dictionaryV1.spec.ts b/packages/node-core/src/indexer/dictionary/v1/dictionaryV1.spec.ts index dadb3ad72b..df8cbbff1e 100644 --- a/packages/node-core/src/indexer/dictionary/v1/dictionaryV1.spec.ts +++ b/packages/node-core/src/indexer/dictionary/v1/dictionaryV1.spec.ts @@ -342,11 +342,23 @@ describe('Dictionary V1 queries', () => { fetchSpy = jest.spyOn(global, 'fetch').mockResolvedValue(new Response('Forbidden', {status: 403})); await expect(queryableDictionary().runQuery('query{_metadata{lastProcessedHeight}}')).rejects.toThrow( - 'Dictionary returned HTTP 403 without a JSON body' + 'Dictionary returned HTTP 403 without a JSON body: Forbidden' ); }); - it('retries a query without distinct when the dictionary does not support it', async () => { + it.each([null, {data: null}, {data: []}])('throws when the dictionary answers %j', async (body) => { + fetchSpy = jest.spyOn(global, 'fetch').mockResolvedValue(jsonResponse(body)); + + await expect(queryableDictionary().runQuery('query{_metadata{lastProcessedHeight}}')).rejects.toThrow( + 'Dictionary returned HTTP 200 without data' + ); + }); + + it.each<[string, () => Response]>([ + ['GraphQL errors', () => jsonResponse({errors: distinctUnsupported}, 400)], + ['errors that are not an array', () => jsonResponse({errors: distinctUnsupported[0].message}, 400)], + ['a plain-text body', () => new Response(distinctUnsupported[0].message, {status: 400})], + ])('retries a query without distinct when the dictionary reports it unsupported in %s', async (_, unsupported) => { const queries: string[] = []; fetchSpy = jest.spyOn(global, 'fetch').mockImplementation((_url, init) => { const {query} = JSON.parse(String(init?.body)) as {query: string}; @@ -355,7 +367,7 @@ describe('Dictionary V1 queries', () => { return Promise.resolve(jsonResponse({data: {_metadata: metadata}})); } if (query.includes('distinct')) { - return Promise.resolve(jsonResponse({errors: distinctUnsupported}, 400)); + return Promise.resolve(unsupported()); } return Promise.resolve( jsonResponse({data: {_metadata: metadata, events: {nodes: [{blockHeight: '150'}, {blockHeight: '170'}]}}}) @@ -369,4 +381,34 @@ describe('Dictionary V1 queries', () => { const batchQueries = queries.filter((query) => query.includes('events')); expect(batchQueries.map((query) => query.includes('distinct'))).toEqual([true, false]); }); + + it('aborts a request after callers have timed out on it', async () => { + let signal: AbortSignal | undefined; + fetchSpy = jest.spyOn(global, 'fetch').mockImplementation( + (_url, init) => + new Promise((_resolve, reject) => { + signal = init?.signal ?? undefined; + signal?.addEventListener('abort', () => reject(new Error('aborted'))); + }) + ); + const dictionary = new QueryableDictionaryV1( + DICTIONARY_ENDPOINT, + DICTIONARY_CHAINID, + new NodeConfig({ + subquery: 'asdf', + subqueryName: 'asdf', + networkEndpoint: {'wss://polkadot.api.onfinality.io/public-ws': {}}, + dictionaryTimeout: 0.1, + }), + HAPPY_PATH_CONDITIONS + ); + + // Callers still get `timeout()`'s error, before the request is aborted + await expect((dictionary as any).init()).rejects.toThrow('Dictionary metadata query timeout in 0.1 seconds'); + assert(signal); + expect(signal.aborted).toBe(false); + + await new Promise((resolve) => signal?.addEventListener('abort', resolve)); + expect(signal.reason).toMatchObject({name: 'TimeoutError'}); + }); }); diff --git a/packages/node-core/src/indexer/dictionary/v1/dictionaryV1.ts b/packages/node-core/src/indexer/dictionary/v1/dictionaryV1.ts index 19b68a8b13..13ed6f59a5 100644 --- a/packages/node-core/src/indexer/dictionary/v1/dictionaryV1.ts +++ b/packages/node-core/src/indexer/dictionary/v1/dictionaryV1.ts @@ -19,14 +19,16 @@ type GraphQLError = {message: string; [key: string]: unknown}; type DictionaryV1BatchResult = {_metadata: DictionaryV1Metadata} & Record; /** - * A dictionary query that failed. `graphQLErrors` holds the errors the dictionary returned, if any, - * which callers inspect to tell an unsupported argument from a failed request. + * A dictionary query that failed. `graphQLErrors` holds the errors the dictionary returned, if any, and `body` the + * response body when it isn't a GraphQL error response. Callers inspect both to tell an unsupported argument from a + * failed request. */ export class DictionaryQueryError extends Error { constructor( message: string, readonly status: number, - readonly graphQLErrors: GraphQLError[] = [] + readonly graphQLErrors: GraphQLError[] = [], + readonly body?: unknown ) { super(message); this.name = 'DictionaryQueryError'; @@ -63,30 +65,43 @@ export abstract class DictionaryV1 extends CoreDictionary< method: 'POST', headers: {'content-type': 'application/json', accept: 'application/json'}, body: JSON.stringify({query, variables}), + // Callers give up on a query through `timeout()`, which doesn't cancel it. Abort it a second later so it doesn't + // keep its connection and body, while callers still get `timeout()`'s error. + signal: AbortSignal.timeout((this.nodeConfig.dictionaryTimeout + 1) * 1000), }); - let body: {data?: T; errors?: GraphQLError[]}; + const text = await response.text(); + let body: {data?: unknown; errors?: unknown} | null; try { - body = (await response.json()) as typeof body; + body = JSON.parse(text); } catch { throw new DictionaryQueryError( - `Dictionary returned HTTP ${response.status} without a JSON body`, - response.status + `Dictionary returned HTTP ${response.status} without a JSON body: ${text.slice(0, 500)}`, + response.status, + [], + text ); } - if (body.errors?.length) { + const errors = body?.errors; + if (Array.isArray(errors) && errors.length) { throw new DictionaryQueryError( - `Dictionary query failed: ${body.errors.map((e) => e.message).join('; ')}`, + `Dictionary query failed: ${errors.map((e) => e?.message).join('; ')}`, response.status, - body.errors + errors ); } - if (!response.ok || body.data === undefined) { - throw new DictionaryQueryError(`Dictionary returned HTTP ${response.status} without data`, response.status); + const data = body?.data; + if (!response.ok || typeof data !== 'object' || data === null || Array.isArray(data)) { + throw new DictionaryQueryError( + `Dictionary returned HTTP ${response.status} without data: ${text.slice(0, 500)}`, + response.status, + [], + body + ); } - return body.data; + return data as T; } protected async init(): Promise {