rtomayko / posix-spawn

Ruby process spawning library
Other
519 stars 52 forks source link

Spawn::Child fails on win32 + 1.9.2 #17

Closed tmm1 closed 9 years ago

tmm1 commented 13 years ago

Looks like write_nonblock fails, even though write works. Probably a ruby bug.

$ ruby -e ' require "posix-spawn"; puts POSIX::Spawn::Child.new("dir",:input=>"
").out '
c:/Ruby192/lib/ruby/gems/1.9.1/gems/posix-spawn-0.3.5.2/lib/posix/spawn/child.rb:179:in `write_nonblock': Bad file descriptor (Errno::EBADF)
        from c:/Ruby192/lib/ruby/gems/1.9.1/gems/posix-spawn-0.3.5.2/lib/posix/spawn/child.rb:179:in `block in read_and_write'
        from c:/Ruby192/lib/ruby/gems/1.9.1/gems/posix-spawn-0.3.5.2/lib/posix/spawn/child.rb:176:in `each'
        from c:/Ruby192/lib/ruby/gems/1.9.1/gems/posix-spawn-0.3.5.2/lib/posix/spawn/child.rb:176:in `read_and_write'
        from c:/Ruby192/lib/ruby/gems/1.9.1/gems/posix-spawn-0.3.5.2/lib/posix/spawn/child.rb:108:in `exec!'
        from c:/Ruby192/lib/ruby/gems/1.9.1/gems/posix-spawn-0.3.5.2/lib/posix/spawn/child.rb:80:in `initialize'
        from -e:1:in `new'
        from -e:1:in `<main>'

$ ruby -ve 'r,w=IO.pipe; Process.spawn("dir", :in => r, :out => "file"); w.write_nonblock("test")'
ruby 1.9.2p180 (2011-02-18) [i386-mingw32]
-e:1:in `write_nonblock': Bad file descriptor (Errno::EBADF)
        from -e:1:in `<main>'
tmm1 commented 13 years ago

Apparently this is documented behavior for IO#write_nonblock, since pipes on win32 are crippled.

We can call write instead of write_nonblock on win32. Not sure there are any other options. @rtomayko: thoughts?

rtomayko commented 13 years ago

Hmm, it's kind of embarrassing how few fucks I give about Windows support to be honest. I don't even like the idea of a simple conditional here. I guess we could load a posix/spawn/windows.rb that replaces methods of Child.

tmm1 commented 13 years ago

Yea that might be better. The whole point of read_and_write is to do non-blocking i/o on the pipes. We're probably better off doing something much more simple/stupid on win32.

jonforums commented 13 years ago

Why not shoe horn in the old albino behavior into the neutered Child methods of posix/spawn/windows.rb instead of spending too much time on an optimization clearly targeted for nix-like systems?

https://github.com/mojombo/jekyll/blob/v0.10.0/lib/jekyll/albino.rb#L61-69

I just want jekyll 0.11.0 running locally to be able to highlight on Windows again without error. I'm guessing no one currently cares about the posix-spawn resource optimizations on Windows (awesome thing for GH and other nix envs) and if they ever do, they can submit patches.

Of course, both of you are magicians so who knows what you've got up your sleeves ;)

rtomayko commented 13 years ago

Yeah, I'm fine so long as either 1.) the windows support code in child.rb is simple, or 2.) it's moved out into a windows.rb lib so it doesn't get loaded at all under unix.

jonforums commented 13 years ago

What's the status/timing of either of you fixing this?

If NOMFRS, are you still of the view to extract a fix to windows.rb (no matter how simple) is the way to go?

rtomayko commented 13 years ago

I have no plans to look into this very soon. Would definitely look at a patch that kept everything in a windows.rb though.

jonforums commented 13 years ago

OK, I'll work to carve out some time, look into it, and get you patch. Any time sink gotcha's I need to be aware of as I'm learning the code?

rtomayko commented 13 years ago

@jonforums: Honestly, I don't have a lot of guidance, unfortunately. I'm pretty ignorant when it comes to windows APIs. It might be worth looking through Ruby 1.9's Process::spawn implementation. Also, I'm not sure how well select(2) works on windows so the current Process::Spawn::Child implementation may be mostly worthless.

jonforums commented 13 years ago

Let's get this quick hoist-the-fix-to-albino-option out of the way first https://gist.github.com/1166390

I need a shower, but it appears to work on my Win7 32bit running ruby 1.9.4dev (2011-08-24 trunk 33031) [i386-mingw32]

Assume it was cleaned up to remove RbConfig::CONFIG duplication, is Albino the right place for this or should this type of short-circuit fix be driven down somewhere sensible in posix-spawn? Or something completely different?

Frankly, at this point all I want is something functional and reliable across OSs rather than optimal. Anyone wanting a better solution can refactor into a windows.rb of posix-spawn.

Yay, or nay, try again?

UPDATE hell, jumped the gun...just saw there''s jruby stuff in posix-spawn...will test this out with jruby on windows later

UPDATE2 bloody hell for jruby...jekyll -> classifier -> fast-stemmer with fast-stemmer being a native C extension...red-herring...not a posix-spawn issue but don't neuter posix-spawn jruby support as RbConfig::CONFIG['host_os'] == "mswin32" on Win7 32bit

jonforums commented 13 years ago

Hoisting to Albino was a terrible idea; testing this set of patches

https://github.com/rtomayko/posix-spawn/compare/master...jonforums:winmods

on MRI 1.9.4 (working), 1.9.3, 1.8.7 on Win7 32bit and will do a quick check on Arch. Would one of you benchmark to see how much this impacts performance in the GH environment?

works on

193: ruby 1.9.3dev (2011-08-23 revision 33029) [i386-mingw32]
194: ruby 1.9.4dev (2011-08-24 trunk 33031) [i386-mingw32]

fails on

187: ruby 1.8.7 (2011-06-30 patchlevel 352) [i386-mingw32]

for two reasons:

  1. test gem (built via RG 1.8.8) failed to install using RG 1.8.9...likely https://github.com/rubygems/rubygems/pull/121 related

    Invalid gemspec in [C:/ruby187/lib/ruby/gems/1.8/specifications/posix-spawn-0.3.7.alpha.1.gemspec]: Illformed requirement ["#YAML::Syck::DefaultKey:0x391a868 0.7.6"]

  2. Once (1) fixed with manual gemspec edit, HTML page contains this error...known issue https://github.com/rtomayko/posix-spawn/issues/9

    Liquid error: fork() function is unimplemented on this machine

tmm1 commented 13 years ago

Patch looks simple enough. On posix systems, a blocking write will cause deadlocks because of PIPE_BUF limits. Once the read and write end of a process like cat reach PIPE_BUF (typically 8k), any additional writes will block until someone empties the read end. This is the entire purpose of Child- to ensure data is streamed through the child process via non-blocking reads/writes to prevent deadlocks.

Since win32 doesn't support non-blocking writes, we have no choice. I'm not even sure if win32 has a PIPE_BUF limit or how it manifests itself. One possible improvement might be to use a blocking write on chunks of the input instead of blocking on writing the entire input. It should be easy to replicate the deadlock by writing a large input to a process that does some sort of streaming to stdout.

jonforums commented 13 years ago

Thanks for the background posix info; seems you're saying that the simple patch won't deadlock on posix.

I'm still concerned about the performance impact of the conditional on your benchmarks and in the GH infrastructure. If it's truly in a critical path, maybe the conditional should be pulled into Child as an ivar or singleton method to minimize the lookup path?

I like your idea on blocking writes on chunked input. I also think http://en.wikipedia.org/wiki/Overlapped_I/O and http://en.wikipedia.org/wiki/Input_output_completion_port might help, but sadly don't have the time now to prove it. I guess any of this could be done in a windows.rb that implements Child#exec! and Child#read_and_write if needed in the future.

Bottom line...are you OK with adding the patch? Anything else you want me to test or try out on Win7 32bit?

rtomayko commented 9 years ago

Closing as stale and windows support is best effort. Please reopen if there are still simple things we can do to clean this up on windows.