Document/detect incompatibility with apartment under default configuration

[betelgeuse]

https://github.com/graphiti-api/graphiti/blob/a5bbb67b075aaba9e50604d7ef05fb609ad1b91a/lib/graphiti/scope.rb#L53

This use of threads means graphiti is incompatible with any gem that relies on all the SQL statements during a request happening within the same thread.

This design decision should be at least documented in bold and ideally known conflicting gems like apartment detected as the problems surfaced are quite hard to debug (queries returning mysterious results).

[betelgeuse]

Also I think it’s a wrong call to disable it by default in the test environment.

https://github.com/graphiti-api/graphiti/blob/a5bbb67b075aaba9e50604d7ef05fb609ad1b91a/lib/graphiti/railtie.rb#L94

This means it’s likely production where this problem surfaces.

[richmolj]

Sorry to hear you've had such a frustrating issue @betelgeuse 😞

We match Rails defaults for tests. If you enabled concurrency for graphiti in tests, everything would fail because Rails itself doesn't run with concurrency on in test mode (IIRC there were issues with supporting gems as well). If you have a different default that works for you, I'd be happy to take a look (fwiw that line is deprecated, it's here now).

I'm always up for improved documentation though. Maybe a "concurrency" section under Resources? If you have an idea of what you'd like to see, the repo is https://github.com/graphiti-api/graphiti-api.github.io

[betelgeuse]

(fwiw that line is deprecated, it's here now).

Copy paste error for the link?

[betelgeuse]

We match Rails defaults for tests. If you enabled concurrency for graphiti in tests, everything would fail because Rails itself doesn't run with concurrency on in test mode (IIRC there were issues with supporting gems as well).

Can you elaborate here. Active Record is thread safe by default. At least when using PostgreSQL the database.yml has a connection pool of 5 by default also for the test environment.

https://github.com/rails/rails/blob/v6.0.3.1/railties/lib/rails/generators/rails/app/templates/config/databases/postgresql.yml.tt

[richmolj]

Sorry about that, correct link is here.

We're not really talking about thread safety here - you could say Graphiti is also threadsafe by default; this is different than an incompatibility with a library that requires all calls to be run in the same thread.

That said, the issues I remember top of my head are A) rails does not eager load by default in test mode, which is requirement for things to work B) the web server under the hood of rspec request specs (and rails integration tests) is not multi-threaded, or at least not multi-threaded by default (I'm not sure about recent changes in Rails 6, but I think it is the same).

For example, go to our sample app. Run in production mode, you will have no issues with concurrency. Run in test mode with concurrency enabled, you will get errors on sideloading specs, since these are ones that will use another thread (the web server returns the response without waiting for the sideload to resolve). So this is working, correct code that will fail when run with vanilla Rails defaults. You'd have the same issue with other libraries that create multiple threads in a request; this is not a graphiti-specific issue.

[betelgeuse]

That said, the issues I remember top of my head are A) rails does not eager load by default in test mode, which is requirement for things to work

Rails 6 uses zeitwerk as the code loader and it's thread safe.

https://github.com/fxn/zeitwerk

B) the web server under the hood of rspec request specs (and rails integration tests) is not multi-threaded, or at least not multi-threaded by default (I'm not sure about recent changes in Rails 6, but I think it is the same).

Rails 6 system specs use threaded puma by default.

For example, go to our sample app. Run in production mode, you will have no issues with concurrency. Run in test mode with concurrency enabled, you will get errors on sideloading specs, since these are ones that will use another thread (the web server returns the response without waiting for the sideload to resolve). So this is working, correct code that will fail when run with vanilla Rails defaults. You'd have the same issue with other libraries that create multiple threads in a request; this is not a graphiti-specific issue.

Per above I think you are using outdated information.

[richmolj]

Again, thread safety is a different issue than the one that we are talking about. I would also love to see a source that threaded puma is used for tests, as ActionDispatch::Integration::Session is what's referenced under the hood.

How about this: create a new rails app with the appropriate models and write this controller:

class Post < ApplicationRecord
  attr_accessor :concurrent_comment
end

class Comment < ApplicationRecord
end

class PostsController < ApplicationController
  def index
    post = Post.first
    something = lambda do
      post.concurrent_comment = Comment.first
    end
    promise = Concurrent::Promise.execute(&something)
    sleep 0.01 until promise.fulfilled? || promise.rejected?
    render json: { val: post.thing }
  end
end

Note any test will fail as the comment never gets assigned. Run an actual server in production mode and things will work correctly.

If you'd like to continue looking into this issue further, I propose you start there, as the code has nothing to do with graphiti.

[betelgeuse]

Again, thread safety is a different issue than the one that we are talking about. I would also love to see a source that threaded puma is used for tests, as ActionDispatch::Integration::Session is what's referenced under the hood.

When I run Rails 6 system tests I get in console:

Capybara starting Puma...
* Version 4.3.1 , codename: Mysterious Traveller
* Min threads: 0, max threads: 4
* Listening on tcp://127.0.0.1:58307

[betelgeuse]

Note any test will fail as the comment never gets assigned. Run an actual server in production mode and things will work correctly.

Work fine in development mode:

class HomeController < ApplicationController
  def home
    post = Post.first
    something = lambda do
      post.concurrent_comment = Comment.first
    end
    promise = Concurrent::Promise.execute(&something)
    sleep 0.01 until promise.fulfilled? || promise.rejected?
    render json: { val: post.concurrent_comment.id }
  end
end

$ curl http://localhost:3000
{"val":1}

If you'd like to continue looking into this issue further, I propose you start there, as the code has nothing to do with graphiti.

Here's the output from a system test:

require "application_system_test_case"

class HomesTest < ApplicationSystemTestCase
  test 'visiting home' do
    visit '/'
    p page.body
  end
end

$ rails test:system
Run options: --seed 12821

# Running:

Capybara starting Puma...
* Version 4.3.5 , codename: Mysterious Traveller
* Min threads: 0, max threads: 4
* Listening on tcp://127.0.0.1:60700
"<html><head></head><body><pre style=\"word-wrap: break-word; white-space: pre-wrap;\">{\"val\":298486374}</pre></body></html>"
.

Finished in 3.718704s, 0.2689 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 0 errors, 0 skips

[richmolj]

OK, I am now closer to understanding what you are saying, thanks for bearing with me. There are a couple things going on here.

First, we would expect things to work in development and production mode. We would expect rspec request specs, Rails integration specs, and graphiti-generated specs to all fail (these are all the same thing, really). This is certainly the case for the code above, for all the reasons outlined.

However, Rails 6 introduced new system specs that you might have, independent of Graphiti. And we should run these specific tests with concurrency on. So it's really less about changing the default, and more about a conditional ("run with concurrency off, unless we are running a system test"). If you'd like to submit a PR that enables concurrency by default only when running system tests and not other kinds of tests I'd be happy to merge and appreciate the help.

If you're instead saying Graphiti should run everything as a system test by default, this has much broader implications (breaking compatibility, slower tests, etc).