First class support for status -1 in Rack::Lint #421

Closed
SamSaffron opened this Issue Aug 24, 2012 · 18 comments

Projects

None yet

3 participants

@SamSaffron

I am sure this has been asked 1000 times before but I could not find it in issues

As rails 4 is making all this async stuff sound so sexy I think it makes sense to offer first class support for async in rack.

The current hack is to return status -1 so the underlying web server knows about the delayed nature of the calls.

Rack::Lint chokes on -1 something that leads to very annoying errors.

I ended up monkey patching it in my app, but it feels wrong.

class ::Rack::Lint
        alias_method :check_status_orig, :check_status
        alias_method :check_headers_orig, :check_headers
        alias_method :check_content_type_orig, :check_content_type
        alias_method :check_content_length_orig, :check_content_length

        def check_status(status)
          @async = true if status == -1
          return if @async
          check_status_orig(status)
        end

        def check_headers(headers)
          return if @async
          check_headers_orig(headers)
        end

        def check_content_type(status,headers)
          return if @async
          check_content_type_orig(status,headers)
        end

        def check_content_length(status,headers)
          return if @async
          check_content_type_orig(status,headers)
        end

      end

I know people mention the whole throw :async thing, but to be honest it actually feels messier than the -1 status.

@raggi raggi was assigned Aug 26, 2012
@rkh
Member
rkh commented Aug 29, 2012

I'd rather have #callback and #errback be part of the spec than async.callback. Status -1 and throw :async are mostly used to bypass Rack.

@SamSaffron

@rkh I may be missing something but the rack stack still needs to be unwound somehow so evented web servers can schedule the next request, what would one return from call, if it is not a throw :async or -1?

@rkh
Member
rkh commented Aug 29, 2012

No, you can just return. I found that in most use cases status code and headers are not set in an evented manner anyways, only the body. This is what Sinatra's stream implementation is doing.

@SamSaffron

@rkh sorry I am being a bit slow, not following

def call(env)
  # surely this is going to wreak more havoc 
  return
end

... caller

def call(env)
   .... 
   # need to handle body.nil? headers.nil? and status.nil? 
end
@rkh
Member
rkh commented Aug 29, 2012

Instead of:

def call(env)
  EM.next_tick do
    env['async.callback'].call [200, {'Content-Type' => 'text/event-stream'}, Stream.new]
  end
  [-1, {}, []]
end

This:

def call(env)
  [200, {'Content-Type' => 'text/event-stream'}, Stream.new]
end

It also makes intermediate code (like Sinatra) independent of eventmachine.

@SamSaffron

@rkh I was worried you were going to suggest that.

That type of API paints you in to a corner, even though one could possibly argue you can pre-determine the Content-Type. Pre-determining the status is something that is very binding.

What happens if you long-poll and want to return a 500 back cause something eventually goes wrong? What if you want to redirect?

And then there are web sockets that need a 101 and a carefully crafted response that is served out of the rack stack.

I feel it would be much more beneficial to have an official pattern for telling rack to leave a request alone.

@rkh
Member
rkh commented Aug 30, 2012

True, but usually the status code is not streamed. And the async.callback solution is an evil hack (which is one of the reasons we need a Rack replacement). There is absolutely no point in doing websockets with Rack, as all the servers buffer in the request body completely before firing off the request.

Why not use throw :async to signal a complete "leave a request alone"?

@SamSaffron

@rkh the request body in a websocket scenario is tiny pre-upgrade, its not such a big deal. Once upgraded you are totally bypassing rack anyway, eg: https://github.com/SamSaffron/thin-em-websocket/blob/master/lib/thin-em-websocket.rb which can be further optimised.

The huge advantage of doing websockets in rack is that you only need to run one web server to serve your app, also you can do cookie/session checks simply prior to upgrading, something that gives you more secure channels.

I totally agree the env['async.callback'] feels ultra messy

I don't know I guess I can accept throw :async you can do 10 of thousands of these a second, thin supports it and rainbows.

@SamSaffron

@rkh Rack::Lock then needs some handling for throw :async, very very oddly it only catches Exception and family, would expect other places in the stack are plagued with this

For my socket stuff I actually want to be fairly high in the stack before deciding to go with a socket or not

@rkh
Member
rkh commented Sep 4, 2012

Uhm, giving Rack::Lock the ability to handle throw :async would defeat the purpose of throw :async, no?

@rkh
Member
rkh commented Sep 4, 2012

Also, I'm not trying to shoot you down, I'm just saying what comes to mind. And having Rack::Lint not complain is not the same as officially and fully supporting it.

@SamSaffron
def call(env)
      old, env[FLAG] = env[FLAG], false
      @mutex.lock
      response = @app.call(env)
      response[2] = BodyProxy.new(response[2]) { @mutex.unlock }
      response
    rescue Exception
      @mutex.unlock
      raise
    ensure
      env[FLAG] = old
    end

I dunno @rkh acquiring a mutex and never releasing it is real bad practice, changing rescue Exception to rescue does not feel like a radical idea and is way more correct.

@rkh
Member
rkh commented Sep 4, 2012

Ah, right.

@raggi
Member
raggi commented Sep 4, 2012

If Lint is to support async, then all of the built-in middleware should too. I'm loathed to do this until we have a middleware superclass, and when I have the time to make a major change like that, rack 2 will take some shape instead.

@SamSaffron

@raggi having throw :async work through the built in stack gets us most of the way there anyway, I am fine with that for now.

@raggi
Member
raggi commented Nov 2, 2012

throw :async was a deliberate premeditated hack to hijack the response context. It will never be directly supported by Rack, as in order to continue that, we'll end up with throw/catch in all the middleware.

The -1 status code is slightly more viable, however, it still requires many modifications to middleware to be truly supported. I'm tending toward direct support for a hijack API instead, as this is much less intense on middleware authors, although harder for users.

@raggi
Member
raggi commented Dec 30, 2012

@rkh lets take a look at the hijack api in go's net/http, i think it might be a good half-way api for SPEC, without the larger rebuild of everything. It won't support full async, but would enable websocket pass-through and could clean up some of our outstanding items for streaming. AFAIK it would completely solve @tenderlove's desires.

@raggi
Member
raggi commented Jan 14, 2013

this is being superseded by the hijack api.

@raggi raggi closed this Jan 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment