Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR related to #403 / Thread safety, actors #404

Closed
wants to merge 7 commits into from
Closed

PR related to #403 / Thread safety, actors #404

wants to merge 7 commits into from

Conversation

@fwoeck
Copy link

@fwoeck fwoeck commented Apr 16, 2014

This contains the changes for #403 - particularly the use of celluloid to avoid concurrency problems. There remain some issues (3 failing specs under rbx/mri) - I spread FIXME-marks as reminders:

  • exception handling within the vcr actor and signaling to the caller is not always working as intended
  • the to_proc syntax in VCR.use_cassette(name, &req) is not yet supported
  • celluloid raises the ruby requirements to 1.9.2+ - I didn't test the other supported platforms yet (e.g. jruby)

@myronmarston We could discuss/resolve the open questions in a remote session or asynchronously, just as you prefer.

Celluloid.logger = nil
Celluloid.boot


Extra blank line detected.

@@ -41,6 +41,10 @@ def make_request

it 'records and plays back properly' do
expect { make_request }.to raise_error(Excon::Errors::NotFound)

# FIXME This fails with "There is already a cassette with the same name (with_errors)"

Line is too long. [92/80]

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Apr 18, 2014

Thanks for opening this...and sorry for the radio silence. Finding time to maintain VCR is hard for me with my responsibilities to RSpec :(.

BTW, ignore the houndci comments. I don't remember enabling that but apparently I did. I just disabled it. It's noise at this point.

celluloid raises the ruby requirements to 1.9.2+ - I didn't test the other supported platforms yet (e.g. jruby)

This is definitely a problem...at least, if we want to release this in a 2.x release. VCR follows SemVer, which means we can't cut a 2.x release that breaks compatibility with 1.8.7. I see two basic paths we can take here:

  • We can decide to bump the major version and call the release that includes this 3.0. I've planned to drop 1.8.7 for 3.0, anyway. There's some other stuff to drop in 3.0 (e.g. fakeweb support, support for typhoeus, 0.4, etc). Beyond the stuff to drop I had never really given much thought to what VCR 3.0 might be, and I'm not sure I'm ready to go there just yet.
  • We can find a way to make celluloid an optional dependency (and maybe even make this entire threadsafety approach an opt-in feature). There are some other reasons why having this opt-in could be beneficial. For example, if your test suite has any places where it kills all non-main threads in order to perform cleanup, that could kill the VCR actor thread, right? I've got some test suites like that where I spin up some threads in some tests and to make sure there's no thread leaks (e.g. if a test fails in mid-stream before joining or whatever) I've got some cleanup code that kills all threads but the main one, and I worry that this celluloid approach may not be compatible with that kind of thing.

I'm kind leaning towards door number 2 personally...what do you think?

I don't have time to look more into this at the moment, sadly...but I do plan to at a later time.

BTW, if you want to discuss this stuff in real time, ping me on IRC. I hang out in the RSpec room on freenode most days.

@fwoeck
Copy link
Author

@fwoeck fwoeck commented Apr 24, 2014

I think making the celluloid-support optional is perfectly doable. Concerning the thread-cleanup: right, celluloid spawns some household-threads (i.e. for supervision) - so killing threads in between will cause havoc. I'll push some commits that make the celluloid dependency an opt-in during the next days.

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Apr 24, 2014

Sounds great! Can you close this PR and open a new one with your further changes? The hound comments add so much noise...it'll be easier for me to review on a fresh PR. I've turned off hound so it shouldn't comment again.

@fwoeck
Copy link
Author

@fwoeck fwoeck commented Apr 24, 2014

Yup, I'll do that!

@fwoeck
Copy link
Author

@fwoeck fwoeck commented May 7, 2014

Closed in favor of new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants