Graphql-ruby: Thread unsafe .connection_type

[ajoneil]

Routinely after deploying our site to production, we get a handful of errors like the following:
Duplicate type definition found for name 'DuplicateClusterConnection'
Normally these are just a couple of errors around deploy time, but a recent deploy generated this error for every request.

After some debugging, I believe the issue is due to race conditions in GraphQL::Schema::Member::RelayShortcuts.connection_type: https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/schema/member/relay_shortcuts.rb#L34
We're running puma, which is multithreaded, and the ||= operator is not thread safe. If this connection is used in multiple places in the graph, multiple classes can be generated, and the single definition check here throws an error.

Possible solutions are to either provide thread safety in connection_type, possibly with https://github.com/reidmorrison/sync_attr or similar, or to relax the duplicate checks for the generated connection class. Happy to put up a PR for either of these, if I know which of the options is more desirable. :)

[nekogami]

That bug was mentionned I think in another issue, the workaround meanwhile is to call MySchema.graphql_definition just after your schema declaration to "cheat" the system.

[chevinbrown]

Any more info here? We're running into a similar condition--I'm assuming b/c we're using puma threads in prod...

[rmosolgo]

call MySchema.graphql_definition just after your schema declaration

What happened when you tried that, @chevinbrown ? Did it behave any differently?

I'm hoping to address this in 1.10 by removing the build-schema stage entirely, instead, building it incrementally as configurations are added. But til then, I would expect that work-around to do the job!

[chevinbrown]

@rmosolgo thanks for the followup.
I came across this shortly after this thread: https://github.com/rmosolgo/graphql-ruby/issues/1505

Adding MySchema.graphql_definition at the bottom has prevented it from happening across a few deploys for...at least 4 hours. 😸

For context, this only started happening when we configured puma workers (properly) from 0 concurrency -> 10+ workers. We were threaded before, but I assume it's something to do with forking?
🤷‍♂️

[ajoneil]

@chevinbrown We've added calling graphql_definition in our before_fork block in the puma config, which seems to be a good workaround until this issue is solved.

[chevinbrown]

@ajoneil Thanks for the confirmation--that was on my mind! I'll give that some testing tomorrow and report here for posterity if all is ok.

[chevinbrown]

@ajoneil, this seems to be the most "proper" way to configure with puma:

before_fork do
  MySchema.graphql_definition
end

Can confirm no errors with this after deploys. Thank you!

[mcelicalderon]

I can confirm calling graphql_definition on the before_fork block did help, but a couple of weeks later the error showed up again. I didn't try https://github.com/rmosolgo/graphql-ruby/issues/2018#issuecomment-448886863 yet but looks like it's the most common fix across similar issues. Will try it now.

[rmosolgo]

The _problem_ here is that .graphql_definition is usually deferred until the first query is executed. For servers that start lots of threads, this can cause all kinds of problems.

My solution is to _stop_ deferring the construction of the schema, and instead build it _synchronously_. It's implemented in #2363 and will be released in 1.10.0. You can get it now on 1.10-dev.