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

Use rb_wait_for_single_fd instead of deprecated rb_thread_select #206

Merged
merged 2 commits into from
Jan 9, 2015

Conversation

fcheung
Copy link
Contributor

@fcheung fcheung commented Dec 15, 2014

rb_thread_select has been removed in ruby 2.2 (see #194 for example). The old code will still be used if rb_wait_for_single_fd is not available (which I believe is since 1.9.3)

I've done a quick check that everything still works as intended (linux & os x) when using events, just using the simple.rb example in the god documentation.

There doesn't seem to be any test coverage for this though, so extra pairs of eyes etc. appreciated

@eric
Copy link
Collaborator

eric commented Dec 15, 2014

It looks like we're failing on the travis-ci tests for 2.1. Do you have any idea what's going on there?

@fcheung
Copy link
Contributor Author

fcheung commented Dec 15, 2014

That's a weird one - it didn't look to me as if the extension was even compiled on travis. Will investigate further

@fcheung
Copy link
Contributor Author

fcheung commented Dec 15, 2014

Couldn't reproduce on my home machine but could reproduce it in a linux vm with that particular test seed. it seems like assert_abort is somehow failing to catch the call to abort in test_metrics. Weirdly it doesn't happen if you run those tests on their own.

@fcheung
Copy link
Contributor Author

fcheung commented Dec 15, 2014

That was an order dependant spec failure - tests are now running in a random order. There may well be more of these to flash out

@lukeasrodgers
Copy link
Contributor

I don't write C, but I tested this out on OSX (ruby 1.8.31.8.7, 1.9.3, 2.1.2) and ubuntu (ruby 2.1.2), and it looks good to me. I also ran it against a branch I'm working on with some higher level specs, and it behaved the same as master.

@fake-or-dead
Copy link

wow I just found this bugs happens and hope you test and merge soon 👍

@gen0cide
Copy link

gen0cide commented Jan 9, 2015

Has there been any consideration given to this patch? 2.2.0 is now considered stable and this popular gem is completely dead in the water without it.

Thoughts? Concerns?

@tmm1
Copy link
Collaborator

tmm1 commented Jan 9, 2015

This looks good to me, but I'm not sure who is in-charge of god releases these days. @eric did you do the last gem release?

@eric
Copy link
Collaborator

eric commented Jan 9, 2015

I believe I did. I can release another one if it needs to be.

@eric
Copy link
Collaborator

eric commented Jan 9, 2015

I guess we should add ruby 2.2 to the travis-ci tests.

eric added a commit that referenced this pull request Jan 9, 2015
Use rb_wait_for_single_fd instead of deprecated rb_thread_select
@eric eric merged commit 0703eab into mojombo:master Jan 9, 2015
@eric
Copy link
Collaborator

eric commented Jan 9, 2015

Okay. It's been released:

Successfully registered gem: god (0.13.5)

@gen0cide
Copy link

You guys rock. Seriously. Leaving a comment, going grocery shopping for the week, and coming home to a fix and a push.

Huge thanks all around.

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

Successfully merging this pull request may close these issues.

6 participants