Skip to content

Thread safety? #16

Closed
mperham opened this Issue Jun 18, 2012 · 24 comments

9 participants

@mperham
mperham commented Jun 18, 2012

Is it safe to assume that cocaine is not meant to be used in a thread safe environment? with_modified_environment looks unsafe.

@jyurek
thoughtbot, inc. member
jyurek commented Jun 18, 2012

Now that you mention it, yeah, that's probably unsafe. I would love ideas on how to make it safe, though. This method of altering the environment for the next command is otherwise vastly superior to setting variables on the command line.

@mperham
mperham commented Jun 18, 2012

I understand but a process's environment is effectively one large global variable. You'll need to consider alternatives.

@jyurek
thoughtbot, inc. member
jyurek commented Jun 19, 2012

Yes, I will, if I want it to be thread-safe. I would love to hear alternatives if you need it to be thread safe. Pull requests are even more appreciated.

@mperham
mperham commented Jun 19, 2012

I must admit that I don't understand why you are manipulating the environment. Can you document how I can set up the process's environment ahead of time so that the modifications are not necessary? Would that work?

@jyurek
thoughtbot, inc. member
jyurek commented Jun 19, 2012

I modify the environment so users of the library can modify the environment for the commands they run. Most often this means modifying the PATH, but anything that users environment variables can be affected. The only other way I know of would be to modify the command line itself, but there are text problems with that as well as it not working well (or at all, don't recall specifically) on Windows.

@mateomurphy

What about prepending an env command to calls, or is that what you mean by "modifying the command line"?

@jyurek
thoughtbot, inc. member
jyurek commented Jun 25, 2012

That is, in fact, what I mean. It's doable on UNIXy systems, but not in Windows. Well, I haven't found a way in windows, though I'd gladly welcome to be shown otherwise.

@mike-burns
thoughtbot, inc. member

How about dropping support for Windows? Or, only being thread-safe on unix?

@jyurek
thoughtbot, inc. member
jyurek commented Jun 29, 2012

I'm currently implementing a solution using Process.spawn and POSIX::Spawn that should be thread safe on 1.9 and gracefully degrade on 1.8. I'll update this as commits warrant.

@jyurek
thoughtbot, inc. member
jyurek commented Jun 29, 2012

Ok, 94dbfa3 contains a bunch of work towards this. While 1.8 will never be thread safe, it'll be usable, which is often enough. 1.9 with and without POSIX::Spawn should be thread safe, and I would absolutely love a repeatable test case that demonstrates the race condition.

Thanks for the report, @mperham!

@jyurek jyurek closed this Jun 29, 2012
@mateomurphy

Great, thanks for this; I'm running 1.9 so this solution is perfect for me.

@mperham
mperham commented Jul 24, 2012

@jyurek It makes me really happy to see this work, thanks so much. Any chance of a release sometime soon? As soon as there's a public release, I can take cocaine and paperclip off of Sidekiq's "troublesome gems" list. I know of at least two users who are using sidekiq and paperclip together and need this.

@geoffw8
geoffw8 commented Jul 24, 2012

@mperham +1

I believe I'm one of those two users. In the meantime though, is there a way I can use it before its released? Point my gemfile at a ref or something?

@ryansch
ryansch commented Jul 24, 2012

I've been a silent watcher. We're using paperclip and evaluating sidekiq for production use. This is one of our pain points.

@mperham
mperham commented Jul 24, 2012

@geoffw8 We're the other user. ;-) We serialize our image processing (one big batch job) in Sidekiq to avoid the issue.

@jyurek
thoughtbot, inc. member
jyurek commented Jul 24, 2012

Given no other issues have come in, I think we can cut a 1.0 release soon. If you want to use this before it's released, you can put this in your Gemfile:

gem :cocaine, :git => "http://github.com/thoughtbot/cocaine.git", :ref => "26a64a9d67a6fcac76349c13167dcddb6596f702"

But that should be considered a temporary measure.

@bnorton
bnorton commented Sep 26, 2012

Just wanted to check back in on this. We use paperclip for publishing custom images to facebook and do so via sidekiq.

@jyurek jyurek reopened this Sep 26, 2012
@jyurek
thoughtbot, inc. member
jyurek commented Sep 26, 2012

In an effort to get some more eyes on this from probably-affected users, I'm reopening this issue. @geoffw8 reported issues with the fix that was put forth here. I fixed what might be a related issue and I'd like to see if it fixes the problem completely.

So, if any of you can try putting gem 'cocaine', :git => "git://github.com/thoughtbot/cocaine.git", :branch => "v0.3" in your Gemfiles and running through your processes, I'd appreciate it.

The change here is handling processes that can/will block. Hopefully none of you see issues after this.

@jyurek
thoughtbot, inc. member
jyurek commented Oct 1, 2012

I've released v0.3.2 that covers these changes, which may be easier to specify. I'd like to see some confirmation that it handles the problem before I close this issue, if someone can confirm that.

@jyurek
thoughtbot, inc. member
jyurek commented Oct 12, 2012

I'm going to close this. If it pops back up or it's still a problem please let me know.

@jyurek jyurek closed this Oct 12, 2012
@aosalias

I'm going to be testing this rather heavily. Will report back with results.

@jyurek
thoughtbot, inc. member
jyurek commented Oct 12, 2012

Excellent, I hope it holds up for you!

@balexand

In ba7630a, the thread-unsafe with_modified_environment method was added back. This bug should be reopened since Cocaine is no longer thread-safe. This lack of thread-safety is the cause of thoughtbot/paperclip#1709.

@balexand

@mperham I've added cocaine and paperclip to the Sidekiq troublesome gems list due to the regression described in the previous comment.

@balexand balexand referenced this issue in mperham/sidekiq Nov 22, 2014
Closed

So, cocaine isn't thread safe - what now? #310

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.