search

thoughtbot / cocaine

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

posix-runner won't accept timeout #81

Closed maxim closed 9 years ago

maxim commented 9 years ago

Currently posix-runner runs the command like this

POSIX::Spawn.spawn(*args)

posix-runner actually supports timeout option, but you can't pass it in like that. You gotta run it like this instead

POSIX::Spawn::Child.new(*args)

Difference being that the latter will buffer all the process output instead of streaming it. This is not a big deal for most use cases.

So in order to make it possible to use timeout in cocaine, I propose one of two solutions, which I can work on: either change current posix-runner, or add a new runner: posix-child-runner. Please give me the green light on one of these and I'll try to submit a patch soon. It will also help me switch Skeptick to use cocaine internally: https://github.com/maxim/skeptick/issues/3

maxim commented 9 years ago

Hi Thoughtbot, this issue just needs a quick reply (solution 1 or 2), and I'll get something written. :)

maxim commented 9 years ago

Hi folks, just checking in to get the green light here. /cc @jyurek

jyurek commented 9 years ago

I think solution 2 is better. Normally I'd say this could easily be its own gem that works alongside cocaine, but I think this is the kind of thing that could be pulled in. I'm going to close this ticket, but only because I expect further discussion on your eventual PR. :+1:

maxim commented 9 years ago

Great, thanks!

maxim commented 9 years ago

Hey @jyurek, after you replied I started exploring, and randomly came across this comment by @HoLyVieR: https://github.com/thoughtbot/cocaine/pull/67#issuecomment-47970859. He mentioned ImageMagick's -limit parameter, which I didn't know existed. My use-case for timeout is really for ImageMagick, so perhaps no point in adding it to cocaine.

At the same time, that PR your wrote adds stdout/stderr capturing, and I now realize that without it I can't use cocaine in Skeptick, since one of the promises is that you get IM errors reproduced as ruby errors. Are you planning to merge your pull request?