Async processing broken when using router about goliath HOT 20 CLOSED

cc: @joshbuddy

from goliath.

I'll take a look at this tonight..

from goliath.

@joshbuddy - I'm the guy that originally reported this issue. If you want to do some remote pairing, that would be super-rad :).

from goliath.

Any updates on this? We need to come up with some sort of workaround for our current release, and ideally it will look somewhat like the eventual solution.

One answer is to mount your API class directly on the route, and let it manage the instantiation of the API instance. e.g.

get '/', SomeApi

vs

get '/' do
  run SomeApi.new
end

from goliath.

Sorry haven't had a chance to dig in, not sure about @joshbuddy either. I think both of the above examples should be equivalent..

from goliath.

I really am hoping to look at this tonight. Sorry for the delay.

from goliath.

+1 for fixing this.

from goliath.

+2

from goliath.

Fixed in 740b601

from goliath.

Is the fix breaking with_api testing? As far as I can tell, on_header is no longer being called.

from goliath.

Yeah, I've broken something ... I will get it sorted out tonight. Either, I'll cut a branch or get everything passing nicely.

from goliath.

Okay, I've had to break the API slightly. My commit is here: 6c30768

The main difference is, if you want to use the on_* methods and mapping, you have to supply the class of the api on the mapping method. So, for the async test, you need to say:

class Test < Goliath::API
  put '/async', AsyncUpload
end

Thoughts anyone?

from goliath.

So, the only change with that is that you don't need to explicitly specify the run ... correct? If so, I actually like this new API much better.

from goliath.

Hmm, question.. with that new API, how do we inject middleware for that endpoint? ex:

  map "/hello_world" do
    use ::Rack::ContentLength
    run HelloWorld.new
  end

from goliath.

Would this fix break the neat bit in examples/rack_routes.rb, too?

map "/:number", :number => /\d+/ do
  if params[:number].to_i > 100
    run BigNumber.new
  else
    run HelloNumber.new
  end
end

from goliath.

@igrigorik: This example would still work as is. The only difference is if you want the on_* handlers to work, you have to supply the class and not use the run command at the end. So, you could also have:

map "/hello_world", HelloWorld do
  use ::Rack::ContentLength
end

@koudelka This example would work normally. This is kind of rabbit hole we've gotten into with this api though. I think the ultimate solution is to build the event handling in in a more clever way. Currently there is a "tacked on" feeling to the whole thing, but I think this is reasonable for now.

from goliath.

I, for one, am fine with this slight asymmetry (see my initial comment)

from goliath.

Well, I'm gonna close this issue, and think about writing a better API another day. I have some ideas of how this would look, so, off to another branch to work on this.

from goliath.

And of course routing is also asymmetric in that passing the API class to map causes middleware to be inherited.

from goliath.

@joshbuddy: Ah, cool, that makes sense.. and works for me, I think. Curious to see what you have in mind for the alternative API.

For sanity, we should still add an integration spec to cover this use case. (and update the routing examples)

from goliath.