From ef8b64d9012ec84e3bada9c9bd46c796a3ea7fc6 Mon Sep 17 00:00:00 2001 From: Jay Herron Date: Tue, 23 Dec 2025 13:56:16 -0600 Subject: [PATCH 1/2] fix: Concurrent error collection blocks correctly Previously, errors would occasionally be underreported, which caused randomly failing tests. This is because when `append(error:` was called concurrently, it occasionally would overwrite a concurrently written `_errorsQueue` --- Sources/GraphQL/Execution/Execute.swift | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Sources/GraphQL/Execution/Execute.swift b/Sources/GraphQL/Execution/Execute.swift index b0a21f8f..dcfc3f07 100644 --- a/Sources/GraphQL/Execution/Execute.swift +++ b/Sources/GraphQL/Execution/Execute.swift @@ -78,7 +78,11 @@ public final class ExecutionContext: @unchecked Sendable { } public func append(error: GraphQLError) { - errors.append(error) + // `append` must explicitly use the DispatchQueue and the underlying storage because by + // default `append` uses separate unblocked get, modify, and replace steps. + errorsQueue.sync(flags: .barrier) { + self._errors.append(error) + } } } @@ -849,7 +853,7 @@ func completeListValue( return try await withThrowingTaskGroup(of: (Int, (any Sendable)?).self) { group in // To preserve order, match size to result, and filter out nils at the end. - var results: [(any Sendable)?] = result.map { _ in nil } + var results = [(any Sendable)?](repeating: nil, count: result.count) for (index, item) in result.enumerated() { group.addTask { // No need to modify the info object containing the path, From 3aa2ed82a4d14a4f394d6d606432075e7d5385c7 Mon Sep 17 00:00:00 2001 From: Jay Herron Date: Tue, 23 Dec 2025 13:58:32 -0600 Subject: [PATCH 2/2] refactor: Removes unnecessary DispatchQueue This is unnecessary because we only ever write the property in its entirety without appending or modifying it in any way. This change may cause us to validate the schema multiple times concurrently early in our start up process, (which should end up with the same result), but the code simplification seems worth it. --- Sources/GraphQL/Type/Schema.swift | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/Sources/GraphQL/Type/Schema.swift b/Sources/GraphQL/Type/Schema.swift index e0bcddbf..95a9989d 100644 --- a/Sources/GraphQL/Type/Schema.swift +++ b/Sources/GraphQL/Type/Schema.swift @@ -34,26 +34,9 @@ public final class GraphQLSchema: @unchecked Sendable { let astNode: SchemaDefinition? let extensionASTNodes: [SchemaExtensionDefinition] - // Used as a cache for validateSchema(). - private var _validationErrors: [GraphQLError]? - private let validationErrorQueue = DispatchQueue( - label: "graphql.schema.validationerrors", - attributes: .concurrent - ) - var validationErrors: [GraphQLError]? { - get { - // Reads can occur concurrently. - return validationErrorQueue.sync { - _validationErrors - } - } - set { - // Writes occur sequentially. - return validationErrorQueue.sync(flags: .barrier) { - self._validationErrors = newValue - } - } - } + /// Used as a cache for validateSchema(). Concurrent access is not protected, so assume + /// it can be changed at any time. + var validationErrors: [GraphQLError]? public let queryType: GraphQLObjectType? public let mutationType: GraphQLObjectType? @@ -95,7 +78,7 @@ public final class GraphQLSchema: @unchecked Sendable { extensionASTNodes: [SchemaExtensionDefinition] = [], assumeValid: Bool = false ) throws { - _validationErrors = assumeValid ? [] : nil + validationErrors = assumeValid ? [] : nil self.description = description self.extensions = extensions