[PATCH] close_connection_after_writing only if not deferred, as in cool.io

[PATCH] close_connection_after_writing only if not deferred, as in cool.io

[Lin Jen-Shin (godfat)]

I don't have 100% confidence about this,
but running test suite "t/t0002-graceful.sh"
during my development for EventMachineThreadPool
would need this to properly pass the tests.

I think this would be needed while using
`throw :async' as well?

The patch is also available on Github:
https://github.com/godfat/rainbows/pull/1

>From bd5c38005f44bbe003bd416f7f871f15b0010c85 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat-hOE/xeEBYYIdnm+yROfE0A@public.gmane.org>
Date: Wed, 19 Dec 2012 04:03:55 +0800
Subject: [PATCH] close_connection_after_writing only if not deferred, as in
 cool.io

---
 lib/rainbows/event_machine/client.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/rainbows/event_machine/client.rb
b/lib/rainbows/event_machine/client.rb
index fc0dfe3..ebb6f17 100644
--- a/lib/rainbows/event_machine/client.rb
+++ b/lib/rainbows/event_machine/client.rb
@@ -28,7 +28,7 @@ class Rainbows::EventMachine::Client < EM::Connection

   def quit
     super
-    close_connection_after_writing
+    close_connection_after_writing if nil == @deferred
   end

   def app_call input
-- 
1.8.0.2
_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying

Re: [PATCH] close_connection_after_writing only if not deferred, as in cool.io

[Eric Wong]

"Lin Jen-Shin (godfat)" <godfat-hOE/xeEBYYIdnm+yROfE0A@public.gmane.org> wrote:
> I don't have 100% confidence about this,
> but running test suite "t/t0002-graceful.sh"
> during my development for EventMachineThreadPool

Heh, I gave that a shot way back in the day but never got it working
to my satisfaction.  Perhaps your fix is what is needed...

> would need this to properly pass the tests.
> 
> I think this would be needed while using
> `throw :async' as well?

I don't think so, you just need to set @deferred=nil in a few places
before calling quit.

I've updated the patch and commit message.  Will apply unless
you have objections:

--------------------------------8<----------------------------------

>From 42bf1f6de55b82af46fd8255453036c6582b7f19 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat-hOE/xeEBYYIdnm+yROfE0A@public.gmane.org>
Date: Wed, 19 Dec 2012 04:03:55 +0800
Subject: [PATCH] event_machine: avoid close on deferred response

close_connection_after_writing only if not deferred, as in
cool.io

Deferred responses may buffer more data down the line, so
keep the connection alive if we have a deferred response
body.

[ew: clear @deferred when we really want to quit,
 updated commit message]

Acked-by: Eric Wong <normalperson-rMlxZR9MS24@public.gmane.org>
---
 lib/rainbows/event_machine/client.rb | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/rainbows/event_machine/client.rb b/lib/rainbows/event_machine/client.rb
index fc0dfe3..f3c2070 100644
--- a/lib/rainbows/event_machine/client.rb
+++ b/lib/rainbows/event_machine/client.rb
@@ -28,7 +28,7 @@ class Rainbows::EventMachine::Client < EM::Connection
 
   def quit
     super
-    close_connection_after_writing
+    close_connection_after_writing if nil == @deferred
   end
 
   def app_call input
@@ -48,6 +48,7 @@ class Rainbows::EventMachine::Client < EM::Connection
   def deferred_errback(orig_body)
     @deferred.errback do
       orig_body.close if orig_body.respond_to?(:close)
+      @deferred = nil
       quit
     end
   end
@@ -103,7 +104,8 @@ class Rainbows::EventMachine::Client < EM::Connection
 
   def next!
     @deferred.close if @deferred.respond_to?(:close)
-    @hp.keepalive? ? receive_data(@deferred = nil) : quit
+    @deferred = nil
+    @hp.keepalive? ? receive_data(nil) : quit
   end
 
   def unbind
-- 
Eric Wong
_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying

Re: [PATCH] close_connection_after_writing only if not deferred, as in cool.io

[Lin Jen-Shin (godfat)]

On Wed, Dec 19, 2012 at 5:45 AM, Eric Wong <normalperson-rMlxZR9MS24@public.gmane.org> wrote:
> Heh, I gave that a shot way back in the day but never got it working
> to my satisfaction.  Perhaps your fix is what is needed...

Glad to know that. Hope this time we could reach your satisfaction :P
I have some hard time running the test suites though :(
I can't even pass all tests without any of my patches with:

    make EventMachine

This works better, but still cannot pass everything:

    make -j8 EventMachine

My linux crashed at the moment, and I am too lazy to fix it right now,
thus running tests on my mac. I'll try to make sure that both EventMachine
and EventMachineThreadSpawn fail on the same tests before sending
the patch. Hope this would be good enough.

I'll continue to work on EventMachineThreadPool and EventMachineFiberSpawn
only if EventMachineThreadSpawn works.

>> would need this to properly pass the tests.
>>
>> I think this would be needed while using
>> `throw :async' as well?
>
> I don't think so, you just need to set @deferred=nil in a few places
> before calling quit.

I am not sure if we read it the same way, but what I mean is that
if the application is using `throw :async', we still need the check in
this patch to avoid dropping connections while receiving SIGQUIT.
Thus even if EventMachineThreadSpawn is not included, we still
need that check for regular EventMachine to quit gracefully for
applications use `throw :async'

But anyway, I still don't really understand all the details in Rainbows,
so of course it's very likely that I am simply wrong :P

> I've updated the patch and commit message.  Will apply unless
> you have objections:

Thank you for the corrections and more explanation in the commit log.
I am all for improving it, so credits don't really matter :) You could also
change all the codes and logs in the patch, like fixing my broken English
as well :P
_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying

Re: [PATCH] close_connection_after_writing only if not deferred, as in cool.io

[Eric Wong]

"Lin Jen-Shin (godfat)" <godfat-hOE/xeEBYYIdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Dec 19, 2012 at 5:45 AM, Eric Wong <normalperson-rMlxZR9MS24@public.gmane.org> wrote:
> > Heh, I gave that a shot way back in the day but never got it working
> > to my satisfaction.  Perhaps your fix is what is needed...
> 
> Glad to know that. Hope this time we could reach your satisfaction :P
> I have some hard time running the test suites though :(
> I can't even pass all tests without any of my patches with:
> 
>     make EventMachine
> 
> This works better, but still cannot pass everything:
> 
>     make -j8 EventMachine
> 
> My linux crashed at the moment, and I am too lazy to fix it right now,
> thus running tests on my mac. I'll try to make sure that both EventMachine
> and EventMachineThreadSpawn fail on the same tests before sending
> the patch. Hope this would be good enough.

I've had no trouble running tests with various Linux distros.
It's been a long time since I've tested on FreeBSD, and I've
not tested on any other *BSD.  I cannot support non-Free OSes.

Anything in the *err logs?  The tests should leave them around on
failure.

> I'll continue to work on EventMachineThreadPool and EventMachineFiberSpawn
> only if EventMachineThreadSpawn works.
> 
> >> would need this to properly pass the tests.
> >>
> >> I think this would be needed while using
> >> `throw :async' as well?
> >
> > I don't think so, you just need to set @deferred=nil in a few places
> > before calling quit.
> 
> I am not sure if we read it the same way, but what I mean is that
> if the application is using `throw :async', we still need the check in
> this patch to avoid dropping connections while receiving SIGQUIT.
> Thus even if EventMachineThreadSpawn is not included, we still
> need that check for regular EventMachine to quit gracefully for
> applications use `throw :async'
> 
> But anyway, I still don't really understand all the details in Rainbows,
> so of course it's very likely that I am simply wrong :P

The catch :async will set @deferred=true (obfuscated :x)
I'll commit this to clarify the code:

--- a/lib/rainbows/event_machine/client.rb
+++ b/lib/rainbows/event_machine/client.rb
@@ -38,14 +38,17 @@ class Rainbows::EventMachine::Client < EM::Connection
     @env[ASYNC_CALLBACK] = method(:write_async_response)
     @env[ASYNC_CLOSE] = EM::DefaultDeferrable.new
     status, headers, body = catch(:async) {
       APP.call(@env.merge!(RACK_DEFAULTS))
     }
 
-    (nil == status || -1 == status) ? @deferred = true :
+    if (nil == status || -1 == status)
+      @deferred = true
+    else
       ev_write_response(status, headers, body, @hp.next?)
+    end
   end
 
   def deferred_errback(orig_body)
     @deferred.errback do
       orig_body.close if orig_body.respond_to?(:close)
       @deferred = nil

> > I've updated the patch and commit message.  Will apply unless
> > you have objections:
> 
> Thank you for the corrections and more explanation in the commit log.
> I am all for improving it, so credits don't really matter :) You could also
> change all the codes and logs in the patch, like fixing my broken English
> as well :P

Your actual fix was more important than my cleanups :)
_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying

Re: [PATCH] close_connection_after_writing only if not deferred, as in cool.io

[Lin Jen-Shin (godfat)]

On Wed, Dec 19, 2012 at 7:59 AM, Eric Wong <normalperson-rMlxZR9MS24@public.gmane.org> wrote:
> I've had no trouble running tests with various Linux distros.
> It's been a long time since I've tested on FreeBSD, and I've
> not tested on any other *BSD.  I cannot support non-Free OSes.
>
> Anything in the *err logs?  The tests should leave them around on
> failure.

>From last time I checked, there's nothing left in the log. I would
guess it's because some command line tools behave differently,
but Rainbows should work fine.

Here's what I need to comment out: (3 tests, ab, rack-fiber_pool,
and "Content-Length is set correctly in headers" (I guess it's grep
behave differently. I could try to install GNU's version and try again.)

diff --git a/t/t0012-spurious-wakeups-quiet.sh
b/t/t0012-spurious-wakeups-quiet.sh
index 23557b7..b63870f 100755
--- a/t/t0012-spurious-wakeups-quiet.sh
+++ b/t/t0012-spurious-wakeups-quiet.sh
@@ -10,11 +10,11 @@ then
 	AB=$(PATH=/usr/local/sbin:/usr/sbin:$PATH which ab 2>/dev/null || :)
 fi

-if test -z "$AB"
-then
+#if test -z "$AB"
+#then
 	t_info "skipping $T since 'ab' could not be found"
 	exit 0
-fi
+#fi

 t_plan 4 "quiet spurious wakeups for $model"
diff --git a/t/t0600-rack-fiber_pool.sh b/t/t0600-rack-fiber_pool.sh
index 01f28b5..ca8fcb1 100755
--- a/t/t0600-rack-fiber_pool.sh
+++ b/t/t0600-rack-fiber_pool.sh
@@ -10,7 +10,7 @@ esac

 require_check rack/fiber_pool Rack::FiberPool

-t_plan 7 "basic test with rack-fiber_pool gem"
+t_plan 6 "basic test with rack-fiber_pool gem"

 CONFIG_RU=rack-fiber_pool/app.ru

@@ -42,8 +42,8 @@ t_begin "no errors from curl" && {

 t_begin "no errors in stderr" && check_stderr

-t_begin "ensure we hit 3 separate fibers" && {
-	test x3 = x"$(sort < $curl_out | uniq | wc -l)"
-}
+# t_begin "ensure we hit 3 separate fibers" && {
+# 	test x3 = x"$(sort < $curl_out | uniq | wc -l)"
+# }

 t_done
diff --git a/t/t9001-sendfile-to-path.sh b/t/t9001-sendfile-to-path.sh
index 5a9fdcd..7ca450a 100755
--- a/t/t9001-sendfile-to-path.sh
+++ b/t/t9001-sendfile-to-path.sh
@@ -2,7 +2,7 @@
 . ./test-lib.sh
 skip_models StreamResponseEpoll

-t_plan 7 "Sendfile middleware test for $model"
+t_plan 6 "Sendfile middleware test for $model"

 t_begin "configure and start" && {
 	rtmpfiles curl_err
@@ -39,9 +39,9 @@ t_begin "X-Sendfile does not show up in headers" && {
 	fi
 }

-t_begin "Content-Length is set correctly in headers" && {
-	expect=$(wc -c < random_blob)
-	grep "^< Content-Length: $expect" $curl_err
-}
+# t_begin "Content-Length is set correctly in headers" && {
+# 	expect=$(wc -c < random_blob)
+# 	grep "^< Content-Length: $expect" $curl_err
+# }

 t_done
_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying

Re: [PATCH] close_connection_after_writing only if not deferred, as in cool.io

[Eric Wong]

"Lin Jen-Shin (godfat)" <godfat-hOE/xeEBYYIdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Dec 19, 2012 at 7:59 AM, Eric Wong <normalperson-rMlxZR9MS24@public.gmane.org> wrote:
> > I've had no trouble running tests with various Linux distros.
> > It's been a long time since I've tested on FreeBSD, and I've
> > not tested on any other *BSD.  I cannot support non-Free OSes.
> >
> > Anything in the *err logs?  The tests should leave them around on
> > failure.
> 
> >From last time I checked, there's nothing left in the log. I would
> guess it's because some command line tools behave differently,
> but Rainbows should work fine.
> 
> Here's what I need to comment out: (3 tests, ab, rack-fiber_pool,
> and "Content-Length is set correctly in headers" (I guess it's grep
> behave differently. I could try to install GNU's version and try again.)

I definitely don't want to rely on GNU-isms in shell scripts,
it should rely on POSIX behavior.

It /should/ work on *BSD, I'll try again on FreeBSD tomorrow.
_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying

Re: [PATCH] close_connection_after_writing only if not deferred, as in cool.io

[Lin Jen-Shin (godfat)]

On Fri, Dec 28, 2012 at 4:43 PM, Eric Wong <normalperson-rMlxZR9MS24@public.gmane.org> wrote:
> I definitely don't want to rely on GNU-isms in shell scripts,
> it should rely on POSIX behavior.
>
> It /should/ work on *BSD, I'll try again on FreeBSD tomorrow.

I got managed to get them all passed now :D
The problem is at `wc`. On my mac, wc would print
some leading spaces even if you use `wc -c < random_blob`.

$ echo 's' | wc -c
       2

After using coreutils from GNU or use sed to strip the spaces,
everything passed, except for the one for Apache ab. I've heard
that ab is broken on mac, and after install ab from:
https://github.com/Homebrew/homebrew-dupes/pull/102
Tests using ab are all passed too.
ref: http://simon.heimlicher.com/articles/2012/07/08/fix-apache-bench-ab-on-os-x-lion

Nothing to do with grep.

Also, after confirming all tests are fine, the only test which cannot
pass for eventmachine with threads is:

    "send big pipelined identity requests"

As you mentioned before, the issue is that there is no easy way to
disable a connection and enable it later on? I tried `pause' and
`resume', but it doesn't seem to work correctly? Or I might have
done something wrong or missed something.

If I took out this line to make Rainbows! buffer everything, it would
also work.

    @_io.shutdown(Socket::SHUT_RD) if @buf.size > 0x1c000

But I guess we shouldn't really buffer them in Ruby. I am still trying
to solve this :/ You can find my working copy here:
https://github.com/godfat/rainbows/pull/2/files
I don't paste it inline here because it's large and I guess we should
make all tests pass. Or we could skip that test for this model?

Another concern is that, the most straightforward implementation
would be subclass Rainbows::EventMachine::Client to make it
app_call in a thread. But there's em_client_class option which
users might provide their own eventmachine connection.

If I did this in a client class, then users who provide their own
client class won't really respect the threads, making using
EventMachine or EventMachineThreadSpawn basically the same.
I think this is somehow unexpected.

But then I realized using another option for threads or not really
complicates the original implementation, this is not what I really
want either. I am not sure if there's a perfect solution for this,
so I just concentrate on passing all tests first...
_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying

Re: [PATCH] close_connection_after_writing only if not deferred, as in cool.io

[Eric Wong]

"Lin Jen-Shin (godfat)" <godfat-hOE/xeEBYYIdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Dec 28, 2012 at 4:43 PM, Eric Wong <normalperson-rMlxZR9MS24@public.gmane.org> wrote:
> > I definitely don't want to rely on GNU-isms in shell scripts,
> > it should rely on POSIX behavior.
> >
> > It /should/ work on *BSD, I'll try again on FreeBSD tomorrow.
> 
> I got managed to get them all passed now :D
> The problem is at `wc`. On my mac, wc would print
> some leading spaces even if you use `wc -c < random_blob`.
> 
> $ echo 's' | wc -c
>        2
> 
> After using coreutils from GNU or use sed to strip the spaces,
> everything passed, except for the one for Apache ab. I've heard

Ah, probably good to add a byte_count() function to test-lib.sh
to get around this...  FreeBSD 9.0 does this, too.
I didn't manage to work more on FreeBSD, got distracted by bugs
in other projects...

> Also, after confirming all tests are fine, the only test which cannot
> pass for eventmachine with threads is:
> 
>     "send big pipelined identity requests"
> 
> As you mentioned before, the issue is that there is no easy way to
> disable a connection and enable it later on? I tried `pause' and
> `resume', but it doesn't seem to work correctly? Or I might have
> done something wrong or missed something.

I think that was another issue I had with the EM thread
implementation I tried...

> If I took out this line to make Rainbows! buffer everything, it would
> also work.
> 
>     @_io.shutdown(Socket::SHUT_RD) if @buf.size > 0x1c000
> 
> But I guess we shouldn't really buffer them in Ruby. I am still trying
> to solve this :/ You can find my working copy here:
> https://github.com/godfat/rainbows/pull/2/files
> I don't paste it inline here because it's large and I guess we should
> make all tests pass. Or we could skip that test for this model?

Tests for features are fine to skip, but being open to such a
trivial DoS is not fine...  Perhaps disable keepalive support
entirely?  (you'd have to disable a lot of tests, perhaps
like StreamResponseEpoll :x)

> Another concern is that, the most straightforward implementation
> would be subclass Rainbows::EventMachine::Client to make it
> app_call in a thread. But there's em_client_class option which
> users might provide their own eventmachine connection.

That was for Cramp, I can't remember exactly how it worked...

> If I did this in a client class, then users who provide their own
> client class won't really respect the threads, making using
> EventMachine or EventMachineThreadSpawn basically the same.
> I think this is somehow unexpected.

They'd have to pick the threaded concurrency model.  Maybe
more Ruby code is thread-safe nowadays...

> But then I realized using another option for threads or not really
> complicates the original implementation, this is not what I really
> want either. I am not sure if there's a perfect solution for this,
> so I just concentrate on passing all tests first...

Yes, Rainbows! already has so many options it's hard to keep
things straight :x

I believe EM is one of the concurrency options people actually use in
Rainbows!, so it's important to not break it.  I don't mind copy+pasting
first and eventually factoring out the common parts if possible.
_______________________________________________
Rainbows! mailing list - rainbows-talk-GrnCvJ7WPxnNLxjTenLetw@public.gmane.org
http://rubyforge.org/mailman/listinfo/rainbows-talk
Do not quote signatures (like this one) or top post when replying