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.

Sign up for GitHub

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

Jump to bottom

Fix memory leak in buildSchema/extendSchema #1417

Merged
merged 2 commits into from
Jul 16, 2018
Merged

Fix memory leak in buildSchema/extendSchema #1417

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67162e5
Fix memory leak in buildSchema/extendSchema
IvanGoncharov Jul 13, 2018
360fac8
Merge branch 'master' into fixMemoryLeak
mjmahone Jul 16, 2018
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
2 changes: 1 addition & 1 deletion .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
"rules": {
"prettier/prettier": 2,

"flowtype/space-after-type-colon": [2, "always"],
Copy link
Member Author

Choose a reason for hiding this comment

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

Required because eslint and prettier disagree about this rule.

"flowtype/space-after-type-colon": 0,
"flowtype/space-before-type-colon": [2, "never"],
"flowtype/space-before-generic-bracket": [2, "never"],
"flowtype/union-intersection-spacing": [2, "always"],
Expand Down
140 changes: 71 additions & 69 deletions src/type/definition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,17 +649,17 @@ export class GraphQLObjectType {
extensionASTNodes: ?$ReadOnlyArray<ObjectTypeExtensionNode>;
isTypeOf: ?GraphQLIsTypeOfFn<*, *>;

_typeConfig: GraphQLObjectTypeConfig<*, *>;
Copy link
Member Author

Choose a reason for hiding this comment

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

The memory leak happens because _typeConfig contains field and interfaces closures that depend on some internal arrays + in case of extendSchema it also depends on the previous schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice debugging!

_fields: GraphQLFieldMap<*, *>;
_interfaces: Array<GraphQLInterfaceType>;
_fields: Thunk<GraphQLFieldMap<*, *>>;
_interfaces: Thunk<Array<GraphQLInterfaceType>>;

constructor(config: GraphQLObjectTypeConfig<*, *>): void {
this.name = config.name;
this.description = config.description;
this.astNode = config.astNode;
this.extensionASTNodes = config.extensionASTNodes;
this.isTypeOf = config.isTypeOf;
this._typeConfig = config;
this._fields = defineFieldMap.bind(undefined, config);
this._interfaces = defineInterfaces.bind(undefined, config);
invariant(typeof config.name === 'string', 'Must provide name.');
if (config.isTypeOf) {
invariant(
Expand All @@ -670,17 +670,17 @@ export class GraphQLObjectType {
}

getFields(): GraphQLFieldMap<*, *> {
return (
this._fields ||
(this._fields = defineFieldMap(this, this._typeConfig.fields))
);
if (typeof this._fields === 'function') {
this._fields = this._fields();
}
return this._fields;
}

getInterfaces(): Array<GraphQLInterfaceType> {
return (
this._interfaces ||
(this._interfaces = defineInterfaces(this, this._typeConfig.interfaces))
);
if (typeof this._interfaces === 'function') {
this._interfaces = this._interfaces();
}
return this._interfaces;
}

toString(): string {
Expand All @@ -693,26 +693,26 @@ defineToStringTag(GraphQLObjectType);
defineToJSON(GraphQLObjectType);

function defineInterfaces(
type: GraphQLObjectType,
interfacesThunk: Thunk<?Array<GraphQLInterfaceType>>,
config: GraphQLObjectTypeConfig<*, *>,
): Array<GraphQLInterfaceType> {
const interfaces = resolveThunk(interfacesThunk) || [];
const interfaces = resolveThunk(config.interfaces) || [];
invariant(
Array.isArray(interfaces),
`${type.name} interfaces must be an Array or a function which returns ` +
`${config.name} interfaces must be an Array or a function which returns ` +
'an Array.',
);
return interfaces;
}

function defineFieldMap<TSource, TContext>(
type: GraphQLNamedType,
fieldsThunk: Thunk<GraphQLFieldConfigMap<TSource, TContext>>,
config:
| GraphQLObjectTypeConfig<TSource, TContext>
| GraphQLInterfaceTypeConfig<TSource, TContext>,
): GraphQLFieldMap<TSource, TContext> {
const fieldMap = resolveThunk(fieldsThunk) || {};
const fieldMap = resolveThunk(config.fields) || {};
invariant(
isPlainObj(fieldMap),
`${type.name} fields must be an object with field names as keys or a ` +
`${config.name} fields must be an object with field names as keys or a ` +
'function which returns such an object.',
);

Expand All @@ -721,12 +721,12 @@ function defineFieldMap<TSource, TContext>(
const fieldConfig = fieldMap[fieldName];
invariant(
isPlainObj(fieldConfig),
`${type.name}.${fieldName} field config must be an object`,
`${config.name}.${fieldName} field config must be an object`,
);
invariant(
!fieldConfig.hasOwnProperty('isDeprecated'),
`${type.name}.${fieldName} should provide "deprecationReason" instead ` +
'of "isDeprecated".',
`${config.name}.${fieldName} should provide "deprecationReason" ` +
'instead of "isDeprecated".',
);
const field = {
...fieldConfig,
Expand All @@ -735,7 +735,7 @@ function defineFieldMap<TSource, TContext>(
};
invariant(
isValidResolver(field.resolve),
`${type.name}.${fieldName} field resolver must be a function if ` +
`${config.name}.${fieldName} field resolver must be a function if ` +
`provided, but got: ${inspect(field.resolve)}.`,
);
const argsConfig = fieldConfig.args;
Expand All @@ -744,7 +744,7 @@ function defineFieldMap<TSource, TContext>(
} else {
invariant(
isPlainObj(argsConfig),
`${type.name}.${fieldName} args must be an object with argument ` +
`${config.name}.${fieldName} args must be an object with argument ` +
'names as keys.',
);
field.args = Object.keys(argsConfig).map(argName => {
Expand Down Expand Up @@ -903,16 +903,15 @@ export class GraphQLInterfaceType {
extensionASTNodes: ?$ReadOnlyArray<InterfaceTypeExtensionNode>;
resolveType: ?GraphQLTypeResolver<*, *>;

_typeConfig: GraphQLInterfaceTypeConfig<*, *>;
_fields: GraphQLFieldMap<*, *>;
_fields: Thunk<GraphQLFieldMap<*, *>>;

constructor(config: GraphQLInterfaceTypeConfig<*, *>): void {
this.name = config.name;
this.description = config.description;
this.astNode = config.astNode;
this.extensionASTNodes = config.extensionASTNodes;
this.resolveType = config.resolveType;
this._typeConfig = config;
this._fields = defineFieldMap.bind(undefined, config);
invariant(typeof config.name === 'string', 'Must provide name.');
if (config.resolveType) {
invariant(
Expand All @@ -923,10 +922,10 @@ export class GraphQLInterfaceType {
}

getFields(): GraphQLFieldMap<*, *> {
return (
this._fields ||
(this._fields = defineFieldMap(this, this._typeConfig.fields))
);
if (typeof this._fields === 'function') {
this._fields = this._fields();
}
return this._fields;
}

toString(): string {
Expand Down Expand Up @@ -982,16 +981,15 @@ export class GraphQLUnionType {
extensionASTNodes: ?$ReadOnlyArray<UnionTypeExtensionNode>;
resolveType: ?GraphQLTypeResolver<*, *>;

_typeConfig: GraphQLUnionTypeConfig<*, *>;
_types: Array<GraphQLObjectType>;
_types: Thunk<Array<GraphQLObjectType>>;

constructor(config: GraphQLUnionTypeConfig<*, *>): void {
this.name = config.name;
this.description = config.description;
this.astNode = config.astNode;
this.extensionASTNodes = config.extensionASTNodes;
this.resolveType = config.resolveType;
this._typeConfig = config;
this._types = defineTypes.bind(undefined, config);
invariant(typeof config.name === 'string', 'Must provide name.');
if (config.resolveType) {
invariant(
Expand All @@ -1002,9 +1000,10 @@ export class GraphQLUnionType {
}

getTypes(): Array<GraphQLObjectType> {
return (
this._types || (this._types = defineTypes(this, this._typeConfig.types))
);
if (typeof this._types === 'function') {
this._types = this._types();
}
return this._types;
}

toString(): string {
Expand All @@ -1017,14 +1016,13 @@ defineToStringTag(GraphQLUnionType);
defineToJSON(GraphQLUnionType);

function defineTypes(
unionType: GraphQLUnionType,
typesThunk: Thunk<Array<GraphQLObjectType>>,
config: GraphQLUnionTypeConfig<*, *>,
): Array<GraphQLObjectType> {
const types = resolveThunk(typesThunk) || [];
const types = resolveThunk(config.types) || [];
invariant(
Array.isArray(types),
'Must provide Array of types or a function which returns ' +
`such an array for Union ${unionType.name}.`,
`such an array for Union ${config.name}.`,
);
return types;
}
Expand Down Expand Up @@ -1216,43 +1214,22 @@ export class GraphQLInputObjectType {
astNode: ?InputObjectTypeDefinitionNode;
extensionASTNodes: ?$ReadOnlyArray<InputObjectTypeExtensionNode>;

_typeConfig: GraphQLInputObjectTypeConfig;
_fields: GraphQLInputFieldMap;
_fields: Thunk<GraphQLInputFieldMap>;

constructor(config: GraphQLInputObjectTypeConfig): void {
this.name = config.name;
this.description = config.description;
this.astNode = config.astNode;
this.extensionASTNodes = config.extensionASTNodes;
this._typeConfig = config;
this._fields = defineInputFieldMap.bind(undefined, config);
invariant(typeof config.name === 'string', 'Must provide name.');
}

getFields(): GraphQLInputFieldMap {
return this._fields || (this._fields = this._defineFieldMap());
}

_defineFieldMap(): GraphQLInputFieldMap {
const fieldMap: any = resolveThunk(this._typeConfig.fields) || {};
invariant(
isPlainObj(fieldMap),
`${this.name} fields must be an object with field names as keys or a ` +
'function which returns such an object.',
);
const resultFieldMap = Object.create(null);
Object.keys(fieldMap).forEach(fieldName => {
const field = {
...fieldMap[fieldName],
name: fieldName,
};
invariant(
!field.hasOwnProperty('resolve'),
`${this.name}.${fieldName} field type has a resolve property, but ` +
'Input Types cannot define resolvers.',
);
resultFieldMap[fieldName] = field;
});
return resultFieldMap;
if (typeof this._fields === 'function') {
this._fields = this._fields();
}
return this._fields;
}

toString(): string {
Expand All @@ -1264,6 +1241,31 @@ export class GraphQLInputObjectType {
defineToStringTag(GraphQLInputObjectType);
defineToJSON(GraphQLInputObjectType);

function defineInputFieldMap(
config: GraphQLInputObjectTypeConfig,
): GraphQLInputFieldMap {
const fieldMap: any = resolveThunk(config.fields) || {};
invariant(
isPlainObj(fieldMap),
`${config.name} fields must be an object with field names as keys or a ` +
'function which returns such an object.',
);
const resultFieldMap = Object.create(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why Object.create(null) instead of {}?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mjmahone I just move code from _defineFieldMap here no changes were made.
But here is the gist on why graphql-js always uses Object.create(null):

const a = {};
Boolean(a.toString)
// true
const b = Object.create(null);
Boolean(b.toString)
// false

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense! Thanks for the info, seems good to merge.

Object.keys(fieldMap).forEach(fieldName => {
const field = {
...fieldMap[fieldName],
name: fieldName,
};
invariant(
!field.hasOwnProperty('resolve'),
`${config.name}.${fieldName} field type has a resolve property, but ` +
'Input Types cannot define resolvers.',
);
resultFieldMap[fieldName] = field;
});
return resultFieldMap;
}

export type GraphQLInputObjectTypeConfig = {|
name: string,
fields: Thunk<GraphQLInputFieldConfigMap>,
Expand Down