[RFC] Add support for concurrent specs

[jgaskins]

The move to allow focus: true and before_each in specs opened the door to asynchronous specs since it no longer runs the spec eagerly. This is something I've been wanting for a while because the majority of your time spent in running the test suite for a typical web app is spent in I/O with the DB rather than in CPU time.

If all of your specs are asynchronous, your suite is only as slow as your slowest spec:

require "spec"

describe "somethign async", async: true do
  50.times do |i|
    it "does a thing that takes a long time(#{i})" do
      sleep rand.seconds # Sleep up to 1 second
    end
  end
end

With this PR, this is the result:

$ crystal/bin/crystal async_spec.cr
Using compiled compiler at crystal/.build/crystal
..................................................

Finished in 998.02 milliseconds
50 examples, 0 failures, 0 errors, 0 pending

A few things I had to do to make this happen:

• All Examples are now direct children of the RootContext
  • I have no idea if this was necessary, but it sure made it a lot easier for me to understand.
  • Without this change, async specs that had sync ancestors were not invoked until they were reached. That could probably be worked around, I just wasn't sure how. Maybe the if async checks could handle that.
  • It probably makes the changes to ExampleGroup#run here unnecessary
• An async ExampleGroup makes all its descendants async
  • This is similar to how a focused ExampleGroup runs all its descendants even if they're not focused, letting those descendants "inherit" the focus status

Important notes

A spec suite that contains async specs cannot depend on any global mutable state. For example, you may not be able to run User.count.should eq 1 unless all of your specs run within their own isolated transaction which is rolled back at the end of each example. And even then! :-)

[Sija]

Is it fine to remove this check?

[jgaskins]

It’s actually required. It checks to see that no two it blocks are running at the same time, but that’s literally the point of this PR. 🙂 The check to see whether someone defined an it block inside another it block was changed in this PR to using a running state and raising if you try to add another example while in that state.

[ysbaddaden]

I find the approach a little weird. The async kwarg feels like we want to test asynchronous behavior (instead of running specs concurrently). It also seems like all specs are run concurrently at once, as demonstrated by the example.

I'd instead spawn a configurable number of concurrent runners (parallel with MT enabled) that would take specs to run from a global pool, instead of spawning as many fibers as there are it blocks to call.

[jgaskins]

The async kwarg feels like we want to test asynchronous behavior (instead of running specs concurrently)

The same way that focus: true changes how the tests are run rather than indicating that you’re testing “focused” behavior, async: true changes how the tests are run rather than indicating that we’re testing async behavior.

If you have a better name in mind than async, I’m happy to change it, but I was looking for something concise and descriptive.

It also seems like all specs are run concurrently at once, as demonstrated by the example.

I'd instead spawn a configurable number of concurrent runners (parallel with MT enabled) that would take specs to run from a global pool, instead of spawning as many fibers as there are it blocks to call

Do you have an end goal in mind that this supports?

[jgaskins]

I've renamed async to concurrent here. Does that more accurately reflect the intent of this PR?

[jgaskins]

@ysbaddaden I think your suggestion for a static level of concurrency has merit here. I've been running this against apps using Neo4j as their DB where connections are pretty lightweight, so I didn't see any problems with an unbounded connection count, but I was just putting together an example that uses Postgres that didn't go very well.

Postgres hard-limits you on the number of open connections to 100 in its default configuration because each new connection forks a process in the Postgres server. We probably don't want an unbounded number of Postgres processes in a spec run. 😂

On that note, does anyone have any suggestions on how to do that? I was thinking a -j option (similar to make and bundle):

crystal spec -j6

I still think that all examples will need to be direct children of the RootContext, but maybe instead of having two separate collections (children and concurrent_children), we'd feed all the examples into a Channel in a typical producer/consumer model:

channel = Channel(Example).new(capacity: examples.size)
done_channel = Channel(Nil).new

# Spread the work across `job_count` fibers
job_count.times { spawn consume(channel, done_channel) }

# Put the work into the queue
examples.each { |example| channel.send example }

# Wait for each one to finish
examples.each { done_channel.receive }

[jgaskins]

Still needs cleanup and the specs are currently broken, but I was able to get the -j option working.

Given the following spec, which spans inserting a trivial amount of data into the DB to inserting a lot of data into the DB just to give it something to wait on:

require "./spec_helper"

require "db"
require "pg"

db = DB.open("postgres:///")

db.exec %{CREATE EXTENSION IF NOT EXISTS "uuid-ossp"}
db.exec "DROP TABLE IF EXISTS example_table"
db.exec <<-SQL
  CREATE TABLE example_table (
    id UUID PRIMARY KEY DEFAULT uuid_generate_v4(),
    name TEXT,
    created_at TIMESTAMPTZ NOT NULL DEFAULT now(),
    updated_at TIMESTAMPTZ NOT NULL DEFAULT now()
  )
SQL

describe "somethign we can do concurrently" do
  (1..1000).to_a.shuffle.each do |i|
    it "does a thing that takes a long time(#{i})" do
      i.times do |i|
        db.exec "INSERT INTO example_table (name) VALUES ('Item ' || $1)", i
      end
    end
  end
end

TL;DR

I was able to drop the runtime from 2:32 to 0:27 (~1/5 the runtime).

No concurrency (no -j, 10% CPU)

➜  concurrent_spec_example git:(master) ✗ ../crystal/bin/crystal spec spec/concurrent_spec_example_spec.cr
Using compiled compiler at /Users/jamie/Code/crystal/.build/crystal
........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Finished in 2:34 minutes
1000 examples, 0 failures, 0 errors, 0 pending

-j4 (28% CPU)

➜  concurrent_spec_example git:(master) ✗ ../crystal/bin/crystal spec spec/concurrent_spec_example_spec.cr -j4
Using compiled compiler at /Users/jamie/Code/crystal/.build/crystal
........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Finished in 49.47 seconds
1000 examples, 0 failures, 0 errors, 0 pending

-j6 (43% CPU)

➜  concurrent_spec_example git:(master) ✗ ../crystal/bin/crystal spec spec/concurrent_spec_example_spec.cr -j6
Using compiled compiler at /Users/jamie/Code/crystal/.build/crystal
........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Finished in 37.5 seconds
1000 examples, 0 failures, 0 errors, 0 pending

-j8 (55% CPU)

➜  concurrent_spec_example git:(master) ✗ ../crystal/bin/crystal spec spec/concurrent_spec_example_spec.cr -j8
Using compiled compiler at /Users/jamie/Code/crystal/.build/crystal
ld: warning: directory not found for option '-L/usr/local/Cellar/crystal/0.34.0/embedded/lib'
........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Finished in 27.62 seconds
1000 examples, 0 failures, 0 errors, 0 pending

[jgaskins]

It looks like when I started this, before_all wasn't a thing, which is probably why this breaks on before_all now.

[waj]

This is awesome! The term async is actually familiar to me, because it's also used in Elixir for the same purpose: https://hexdocs.pm/ex_unit/ExUnit.html. The semantics are a little bit different though, because it allows running the test module concurrently with others, but each test is still run serially. I can image scenarios where each test share some common state, so it would be useful to have some level of control over that.

[jgaskins]

Agreed, there are quite a few ways this could go and I honestly don't know which is preferable.

Regarding tests sharing common state, do you mean something like:

• insert record into DB
• run test A
• run test B

… where both A and B operate on the same record in the DB and are expected to run in order?

[jwoertink]

It's been a year, but having just saw this:

...............................................
Finished in 5:57 minutes
47 examples, 0 failures, 0 errors, 0 pending

I was hoping this could be dusted off and looked at again. 😅 What are your thoughts on this @beta-ziliani @straight-shoota

[straight-shoota]

I think this looks good 👍

It needs an update, though. Please merge master.

[straight-shoota]

What is this for?

[jgaskins]

I'm not entirely sure anymore. I wrote this over a year ago. 🙂

I remember I needed something to send a notification of completion to the main fiber. I don't remember why I implemented it this way specifically, though.

[straight-shoota]

Is this actually used? I don't see where.

[jgaskins]

I've had to update this PR several times since it's been opened due to implementation changes in Spec, so I'm not 100% sure which changes were made when and why, but I definitely did use it at one point.

Looks like I used it in the beginning when there was a distinction between sync vs async specs (before I added the -j option).

[straight-shoota]

The new parameter must not be added before any existing positional parameter for backwards compatibility. Please move concurrent to the end. And I would prefer to make it a named parameter. There's not much sense in writing method calls with seven positional arguments.

(I also suggest to make all but description named parameters, but that's a story for another time)

[jgaskins]

And I would prefer to make it a named parameter.

What do you mean? I thought the caller determined whether something was a named parameter.

To be clear, I agree on backwards compatibility (especially since 1.0 is out), but also I wouldn't expect anyone to be writing these positionally and I certainly wouldn't want to work on code that used them that way. 😂

Regardless, IIRC with the -j option the concurrent parameter is no longer be necessary anyway.

[Blacksmoke16]

What do you mean? I thought the caller determined whether something was a named parameter.

You can make it so an argument is required to be provided as a named argument.

When a splat parameter has no name, it means no more positional arguments can be passed, and any following parameters must be passed as named arguments. For example:

# Only one positional argument allowed, y must be passed as a named argument
def foo(x, *, y)
end

foo 1        # Error, missing argument: y
foo 1, 2     # Error: wrong number of arguments (given 2, expected 1)
foo 1, y: 10 # OK

From: https://crystal-lang.org/reference/syntax_and_semantics/default_values_named_arguments_splats_tuples_and_overloading.html#how-call-arguments-are-matched-to-method-parameters.

[jgaskins]

Huh, TIL. I'd always wondered why the nameless splat existed in some stdlib methods.

[jgaskins]

Before we get too deep in the weeds here, there are still open questions about what it means to run concurrent specs. There are multiple implementations in this PR — it "does something", concurrent: true to make certain specs execute outside the main fiber (the original implementation) vs crystal spec -j6 to execute all specs in one of a set number of fibers (added in the most recent commit).

Juan made some observations almost a year ago and I followed up a few hours later trying to get some more insight. Maybe we could start there. Specifically, what is the granularity we're looking at for concurrent specs? IIRC, the current implementation does it at the Example level (it ensures the most even distribution across fibers), but Juan mentioned it may make sense to do it at the ExampleGroup level (I'm assuming for reasons of global state). If we go with that suggestion, what does it mean for nested example groups?

Other questions that could help clarify the larger vision of concurrent specs in Crystal:

• Do we want to run all specs inside the fiber pool (whose size is specified with the -j option) or let some run linearly in the main fiber? The original implementation required specifying concurrent: true (well, originally async: true, but there was an objection to that name) but when I added the -j option I moved everything over to the fiber pool. Is that a good idea? What are the pros/cons?
• What should the default size of the fiber pool be? I think it's currently 1 in this PR, but is that the right value?

[straight-shoota]

Many spec suites probably contain specs that can't run concurrently because they access shared resources. That could be a database, environment variable, a specific file path, non-concurrent API etc. For a concrete example, all stdlib specs using the with_env helper with the same environment variable must not run concurrently.

I assume that such tests would typically be collected in a single example group (if there are other specs in the group, the non-concurrent specs could be filtered to a sub-group). In that scenario, it would be sufficient to configure the example group as running its examples in sequence to avoid concurrency issues.
More problematic are cases where specs in different groups and even different files need to be coordinated. I'm not sure how much this is relevant, but I could imagine use cases for that. Adding some kind of virtual execution groups for coordination would add a lot of complexity.

However, I believe there could be a relatively trivial solution if we operate on very small sections. Files execute sequentially. Every example group can be considered a concurrency context, and defines whether its children execute sequentially or concurrently. This is closely related to the concept of structured concurrency in #6468.

Let's take this example:

# bar_spec.cr
describe "bar" do
  it "A" {}
  it "B" {}
  describe "C" do
    it "1" {}
    it "2" {}
  end
end
# foo_spec.cr
describe "foo" do
  it "A" {}
end

describe "baz" do
  it "A" {}
end

The execution order would be:

• bar, foo, baz execute sequentially
• in bar, bar.A, bar.B, bar.C execute concurrently
• in bar.C, bar.C.1, bar.C.2 execute concurrently

For simplicity, I just assumed that all example groups would be concurrent by default. That does not need to be the case. Either way, it is possible to configure every example group with concurrent: true/false.

Considering that a group usually contains more examples than the concurrency limit and that these examples roughly have a similar execution length, this is probably a reasonable approach.
For more flexibility, there could also be a configuration option to merge the examples of a group in the parent group. If applied on bar.C in the above example, bar.A, bar.B, bar.C.1 and bar.C.2 execute concurrently.