search

thoughtbot / cocaine

A small library for doing (command) lines.
https://robots.thoughtbot.com
Other
785 stars 55 forks source link

Thread safety? #16

Closed mperham closed 12 years ago

mperham commented 12 years ago

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

jyurek commented 12 years ago

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 commented 12 years ago

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

jyurek commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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

jyurek commented 12 years ago

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 commented 12 years ago

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

jyurek commented 12 years ago

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 commented 12 years ago

Ok, 94dbfa336751c11432c6cf0254357fad66802eca 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!

mateomurphy commented 12 years ago

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

mperham commented 12 years ago

@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 commented 12 years ago

@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 commented 12 years ago

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

mperham commented 12 years ago

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

jyurek commented 12 years ago

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 commented 12 years ago

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

jyurek commented 12 years ago

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 commented 12 years ago

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 commented 12 years ago

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

aosalias commented 12 years ago

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

jyurek commented 12 years ago

Excellent, I hope it holds up for you!

balexand commented 9 years ago

In https://github.com/thoughtbot/cocaine/commit/ba7630ab83945a4eb11d3e0a30fe3664bd3d9a84, 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 https://github.com/thoughtbot/paperclip/issues/1709.

balexand commented 9 years ago

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