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

Run batching queries in parallel (#176) #273

Merged
merged 6 commits into from
Jan 24, 2017
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Changelog

### VNEXT
* run batched requests in parallel ([@DxCx](https://github.com/DxCx)) on [#273](https://github.com/apollostack/graphql-server/pull/273)
* Fix GraphiQL options variables. Issue #193. ([@alanchristensen](https://github.com/alanchristensen)) on
[PR #255](https://github.com/apollostack/apollo-server/pull/255)

Expand Down
12 changes: 6 additions & 6 deletions packages/graphql-server-core/src/runHttpQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,7 @@ export async function runHttpQuery(handlerArguments: Array<any>, request: HttpQu
requestPayload = [requestPayload];
}

let responses: Array<ExecutionResult> = [];
for (let requestParams of requestPayload) {
const requests: Array<ExecutionResult> = requestPayload.map(requestParams => {
try {
let query = requestParams.query;
if ( isGetRequest ) {
Expand Down Expand Up @@ -128,17 +127,18 @@ export async function runHttpQuery(handlerArguments: Array<any>, request: HttpQu
params = optionsObject.formatParams(params);
}

responses.push(await runQuery(params));
return runQuery(params);
} catch (e) {
// Populate any HttpQueryError to our handler which should
// convert it to Http Error.
if ( e.name === 'HttpQueryError' ) {
throw e;
return Promise.reject(e);
}

responses.push({ errors: [formatErrorFn(e)] });
return Promise.resolve({ errors: [formatErrorFn(e)] });
}
}
});
const responses = await Promise.all(requests);

if (!isBatch) {
const gqlResponse = responses[0];
Expand Down
8 changes: 6 additions & 2 deletions packages/graphql-server-express/src/apolloServerHttp.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,9 @@ const version = 'modern';
describe(`GraphQL-HTTP (apolloServer) tests for ${version} express`, () => {
describe('POST functionality', () => {

it('allows gzipped POST bodies', async () => {
it('allows gzipped POST bodies', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this change related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node 4 is much slower then Node 6.
I increase those 2 tests timeout since there is timeout after exhausting node abit..

// Increase timeout for slow node 4
this.timeout(3000);
const app = express();

app.use(urlString(), bodyParser.json());
Expand All @@ -181,7 +183,9 @@ describe(`GraphQL-HTTP (apolloServer) tests for ${version} express`, () => {
});
});

it('allows deflated POST bodies', async () => {
it('allows deflated POST bodies', async function () {
// Increase timeout for slow node 4
this.timeout(3000);
const app = express();

app.use(urlString(), bodyParser.json());
Expand Down
35 changes: 35 additions & 0 deletions packages/graphql-server-integration-testsuite/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
GraphQLSchema,
GraphQLObjectType,
GraphQLString,
GraphQLInt,
GraphQLError,
GraphQLNonNull,
introspectionQuery,
Expand All @@ -29,6 +30,17 @@ const queryType = new GraphQLObjectType({
return 'it works';
},
},
testStringWithDelay: {
type: GraphQLString,
args: {
delay: { type: new GraphQLNonNull(GraphQLInt) },
},
resolve(root, args) {
return new Promise((resolve, reject) => {
setTimeout(() => resolve('it works'), args['delay']);
});
},
},
testContext: {
type: GraphQLString,
resolve(_, args, context) {
Expand Down Expand Up @@ -457,6 +469,29 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
});
});

it('can handle batch requests in parallel', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another potential testing strategy that doesn't require the test to take a long time is to have much shorter timeouts in the queries but to expect the part of one query after a timeout to be able to observe the effect of another query.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i thought about creating a promise and resolve it on the other query.
but it felt just too bad..
this is probably what i would've done if it was not going through the whole query engine and such.

// this test will fail due to timeout if running serially.
const parallels = 100;
const delayPerReq = 40;
this.timeout(3000);

app = createApp();
const expected = Array(parallels).fill({
data: { testStringWithDelay: 'it works' },
});
const req = request(app)
.post('/graphql')
.send(Array(parallels).fill({
query: `query test($delay: Int!) { testStringWithDelay(delay: $delay) }`,
operationName: 'test',
variables: { delay: delayPerReq },
}));
return req.then((res) => {
expect(res.status).to.equal(200);
return expect(res.body).to.deep.equal(expected);
});
});

it('clones batch context', () => {
app = createApp({graphqlOptions: {
schema,
Expand Down