search

celluloid / reel

UNMAINTAINED: See celluloid/celluloid#779 - Celluloid::IO-powered web server
https://celluloid.io
MIT License
596 stars 87 forks source link

Recent commit breaks sending Files in response. #87

Closed digitalextremist closed 11 years ago

digitalextremist commented 11 years ago

fea39940f1a828dbfaf55e928134a72d235a1291 seems to have broken responses with Files in them, unless there is a new way of passing those.

digitalextremist commented 11 years ago

It seems this commit breaks jRuby 1.7.4 ... at least the version I have.

Related? jruby/jruby/pull/723 ... or jruby/jruby/pull/683?

digitalextremist commented 11 years ago

if defined?( JRUBY_VERSION ) #de && JRUBY_VERSION <= "1.6.7" fixed it for now... Excited about Celluloid::IO support in the future potentially.

digitalextremist commented 11 years ago

Works: sucks to do, but works: https://github.com/penultimatix/reel/commit/93e56d8528f48a84df64dea7802d81cec33b4743

tarcieri commented 11 years ago

Can you please post a stack trace of the error you're experiencing? JRuby should implement IO.copy_stream for this purpose

digitalextremist commented 11 years ago

Sure thing:

Exception: Should be String or IO
[
  "org/jruby/RubyIO.java:4286:in `copy_stream'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/reel-c209110c65b5/lib/reel/response_writer.rb:47:in `handle_response'",
  "org/jruby/RubyProc.java:255:in `call'",
  "org/jruby/RubyKernel.java:1932:in `public_send'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/calls.rb:25:in `dispatch'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/calls.rb:67:in `dispatch'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/future.rb:14:in `new'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/thread_handle.rb:13:in `initialize'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/thread_handle.rb:12:in `initialize'",
  "org/jruby/RubyProc.java:255:in `call'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/internal_pool.rb:100:in `create'",
  "(celluloid):0:in `remote procedure call'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/future.rb:104:in `value'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/future.rb:68:in `value'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid.rb:452:in `defer'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/reel-c209110c65b5/lib/reel/response_writer.rb:47:in `handle_response'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/reel-c209110c65b5/lib/reel/connection.rb:108:in `respond'",
  "/mu/zero/qi/reaf/rondar/response.rb:25:in `process_response'",
  "/mu/zero/qi/reaf/rondar/skild.rb:20:in `accomodate'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/reel-c209110c65b5/lib/reel/connection.rb:74:in `each_request'",
  "/mu/zero/qi/reaf/rondar/skild.rb:15:in `accomodate'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/calls.rb:25:in `public_send'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/calls.rb:25:in `dispatch'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/calls.rb:67:in `dispatch'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/actor.rb:322:in `handle_message'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/actor.rb:416:in `task'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/tasks.rb:55:in `initialize'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/tasks.rb:47:in `initialize'",
  "/usr/local/rvm/gems/jruby-1.7.4/bundler/gems/celluloid-ad503d06e9b0/lib/celluloid/tasks/task_fiber.rb:13:in `create'"
]
tarcieri commented 11 years ago

@digitalextremist that's very strange. What's getting passed into IO.copy_stream? Could you do an inspect of both parameters before the IO.copy_stream directive on line 47 of Reel's response_writer.rb?

digitalextremist commented 11 years ago

@tarcieri I have done that more superficially, at the point that my code sends it off to Reel's, and it was a File object. I will modify my patch to inspect further --

digitalextremist commented 11 years ago

Inspect pre-copy:

body=#<File:/mu/zero/public/favicon.ico>,
socket=#<Celluloid::IO::TCPSocket:0x48fc989c @read_buffer="", @eof=false, @addr=nil, @write_latch=#<Celluloid::IO::Stream::Latch:0x3f874f33 @condition=#<Celluloid::Condition:0x4c60bdda>, @waiters=0, @owner=nil>, @read_latch=#<Celluloid::IO::Stream::Latch:0x63505a62 @condition=#<Celluloid::Condition:0x305e0ade>, @waiters=0, @owner=nil>, @socket=#<TCPSocket:fd 2632>, @sync=true, @write_buffer="">
digitalextremist commented 11 years ago

I literally had a dream last night that you and @halorgium fixed this :)

tarcieri commented 11 years ago

Would love to fix it. Is it possible JRuby simply isn't converting Celluloid::IO::TCPSocket to the underlying IO class?

digitalextremist commented 11 years ago

How could I test that, or how could I patch that line to convert to the underlying IO class in advance?

tarcieri commented 11 years ago

Well clearly a spec would be super helpful ;) But if you want to try it out yourself:

https://github.com/celluloid/reel/blob/master/lib/reel/response_writer.rb#L47

Change @socket to @socket.to_io

digitalextremist commented 11 years ago

Perfect, thank you. I will try that now.

digitalextremist commented 11 years ago

Worked: submitting patched branch PR //

tarcieri commented 11 years ago

You should totally add a test! :D

Tony Arcieri

On Sep 11, 2013, at 6:45 PM, digitalextremist notifications@github.com wrote:

Worked: submitting patched branch PR //

— Reply to this email directly or view it on GitHubhttps://github.com/celluloid/reel/issues/87#issuecomment-24289704 .

digitalextremist commented 11 years ago

That'd/ll be the first test written in my life and might lead to good behavior. I will learn this ninja skill called 'add test' -- one moment please!

digitalextremist commented 11 years ago

Ok I don't want to be a pain, but could someone write the test for me and I will use this as an example for my future success in obvious and ordinary practices?

If not, this would be in connection_spec.rb correct?

tarcieri commented 11 years ago

I'll write a spec, it's okay. It should probably go in response_writer_spec.rb

digitalextremist commented 11 years ago

Thank you. I look forward to using that as a guide for future specs. Always known this day would come //

tarcieri commented 11 years ago

Spec added in 8c66e4a7631921dd6d41c7ec6ce5c9dbdd8cccb8

tarcieri commented 11 years ago

Hmm, the specs passed without your patch :| Oh well, I'll call this solved

digitalextremist commented 11 years ago

Well the real world fails without my patch. Not sure why. Damn you, real world! Sit. Stay!

digitalextremist commented 11 years ago

Thanks again for your spec, and for including my PR.