Improve the output matcher to make it thread safe

[cupakromer]

The current output matcher is not considered thread safe.

Rails had a similar feature capture and silence which were both recently deprecated. This caused issues with the ammeter gem which relied on those methods. The patch applied there was near identical to how the output matcher works. Based on feedback this is not considered thread safe either.

[mikegee]

This seems to work in my initial testing:

def self.capture(&block)
  reader, writer = IO.pipe

  fork do
    reader.close
    $stdout = writer
    block.call
  end
  writer.close

  reader.read
end

Downsides include:

• decreased portability? (jvm, windows)
• increased overhead
• errors raised in the block would not stop the execution of the parent

[JonRowe]

We can't use fork as far as I know because we support JRuby, windows and I don't think RBX supports it either...

[myronmarston]

fork also has some pretty surprising effects: state mutations in the expect block will not be visible outside of it (as the block will run in a sub process), which is very unexpected behavior. I think fork has too many downsides and gotchas to consider.

[mikegee]

Gotcha. No forking allowed.

Is there a thread-safe implementation of the output matcher? Put me down as pessimistic.

Does the output matcher need to go away in favor of a thread-safe Rspec?

[myronmarston]

Is there a thread-safe implementation of the output matcher? Put me down as pessimistic.

I'm not sure. I haven't looked into this issue at all yet, honestly.

Does the output matcher need to go away in favor of a thread-safe Rspec?

I know minitest has a similar feature, and it has a multithreaded parallel test runner, so it doesn't seem like they are fundamentally incompatible. Also, the matcher is brand new in 3.0...so I don't want to consider removing it. If we simply can't find a way to make it threadsafe, we can (and should) document that this particular matcher isn't threadsafe so users are aware of the issue.

[mikegee]

minitest is using the same thread-unsafe (StringIO, global swapping) implementation: https://github.com/seattlerb/minitest/blob/master/lib/minitest/assertions.rb#L399

[bf4]

Maybe see rubygems, per my comment rails/rails#13392 (comment)

[soulcutter]

I feel like a mutex is best way to guarantee threadsafety (possibly combined with the rubytapas-style capturing mentioned by @bf4 in the other comment), however it's probably important to not initialize the mutex with a non-atomic ||= operation, though the risk of a race condition is small.