Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from Sinatra to a Rack application #3075

Merged
merged 24 commits into from Aug 24, 2016

Conversation

Projects
None yet
4 participants
@badosu
Copy link
Contributor

commented Jul 27, 2016

Continuing discussion from #3069

About extensions:

  • It is not needed anymore to include explicitly home_path on redirect calls, as seen here
  • I had to reinclude code to set local variables inside partials, however It is not compatible with ruby 2.0.0 and I can't see a way to do it without using eval.
  • params includes only the querystring parameters accessible via String keys instead of symbols
  • route_params includes those that come from the route, accessible via symbols
  • The halt call should be replaced to next, e.g. halt 404 -> next 404

We will have to decide what to deprecate and what not, fortunately it looks pretty straightforward to upgrade old extensions to the new model.

badosu and others added some commits Jul 26, 2016

Migrate Sidekiq::Web to a pure Rack application
Migrate Sidekiq::Web a pure Rack application to avoid sinatra as
dependency. rack-protection is still needed.

The application is mounted on top of Rack::Builder, mantaining all of
the previous http interface.

Rack apps being used:

- Rack::File to serve assets
- Rack::Session::Cookie, the secret can be configured via
  Sidekiq::Web.session_secret
- Rack::Protection, same as before when using sinatra
- Sidekiq::WebApplication, described below.

Sidekiq::WebApplication is a very simple rack application composed of a
Sidekiq::WebRouter and a Sidekiq::WebAction dispatcher. This terminology
was adopted to be able to mantain Sidekiq::Web as a Rack app.

The Router is heavily inspired on Rack::Router[0] (and in many parts
identical), however not being retrocompatible.

The Action is a wrapper to provide convenience, DRY code and maintain
the old interface.

I tried to mantain most of the old application structures so that
customizations and monkey-patches are easily adjustable or even
further work be done to enforce retrocompatibility.

Testing welcome!

0: https://github.com/pjb3/rack-router

@badosu badosu changed the title Migrate Sinatra to a Rack application Migrate from Sinatra to a Rack application Jul 27, 2016

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 27, 2016

  • If you can't do it with Ruby 2.0, how does Sinatra do it? Does it use eval?
  • Ok about halt. You should add a halt method that raises an informative error.
@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 27, 2016

I believe Sinatra uses throw/catch to implement halt. Would it be possible to keep the same pattern?

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2016

Do you have a list of known Sidekiq extensions so that I can take a look at what I need to do to support them all?

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 27, 2016

You could look thru the Related Projects wiki page.

On Jul 27, 2016, at 16:46, Amadeus Folego notifications@github.com wrote:

Do you have a list of known Sidekiq extensions so that I can take a look at what I need to do to support them all?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@natebird

This comment has been minimized.

Copy link

commented on 9ea167d Jul 28, 2016

Wow. I expected much more changes than this for a port to Rack. This is very nice. It fits right in with Sidekiq's reduce dependencies motto.

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2016

@mperham After some time thinking about it, I`ve decided to make this change totally compatible with the previous sinatra implementation, these are the major points I've identified so far:

  • The redirect call should not include the custom base path
  • The render method should accept the same arguments as the previous impl.
  • Implement halt
  • params should be an indifferent access hash and also include the params from the route

I'll take a look at this on this weekend. Feel free to push forward if you want as well.

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 28, 2016

That's great, do what you feel is right. A change this big deserves a major version bump, since it'll for sure break someone, but do what you can to minimize the upgrade pain.

@badosu badosu force-pushed the badosu-remove-sinatra branch 5 times, most recently from 1328286 to 5c3a6bd Jul 29, 2016

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2016

Okay, I am almost finished now, besides all those points above I had to:

  • Implement content_type
  • Implement matchers on before and after hooks
  • Change add_to_head
  • erb implementation was trickier than I thought, as it was used for templates with and without layouts depending if it was called inside a template or not
  • Add settings

These web extensions are passing the tests and are looking fine:

Needs further testing and adjust:

Not supported (requires extensive changes):

Not tested yet:

@badosu badosu force-pushed the badosu-remove-sinatra branch from 5c3a6bd to d05066d Jul 29, 2016

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 29, 2016

Wow, you're incredibly thorough. I'm impressed. 🍰

I'll review soon and see what else needs to be done to make it work with Sidekiq Pro and Enterprise's UI extensions.

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 29, 2016

Can you benchmark a few different pages so we can see how performance looks vs Sinatra?

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2016

@mperham I don't know of It'll be a great difference as there aren't any optimizations and also I've sacrificed performance in favour of compatibility with sinatra (see #params implementation)

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 29, 2016

Updated PR with changes to make Sidekiq Pro and Sidekiq Enterprise run green. Notably it took me about an hour to figure out this little tidbit, necessary to run multiple Sidekiq::Web instances in the same process:

http://stackoverflow.com/questions/5643907/what-is-x-cascade-header

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2016

Surprisingly enough, peformance is worse after this change.

Using a config.ru just mounting the application and thin, with ab -l -c 4 -n 2000 http://0.0.0.0:3000/

  • ~400 req/s with Sinatra
  • ~200 req/s new impl.
  • ~300 req/s with a simple ERB cache

Possible causes:

  • The Router uses a regexp match even when it is not needed
  • Tilt can have a better template caching system than the one I am implementing
  • Rack::Builder
@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2016

I just updated the PR with 2 changes:

  • Set content-length header, as I could not even test the application with ab without it and it is a tiny implementation
  • Implement simple caching of erb templates
@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 29, 2016

An easy optimization to make is to split the routes based on method.

On Jul 29, 2016, at 12:15, Amadeus Folego notifications@github.com wrote:

Surprisingly enough, peformance is worse after this change.

Using a config.ru just mounting the application and thin, with ab -l -c 4 -n 2000 http://0.0.0.0:3000/

~400 req/s with Sinatra
~200 req/s new impl.
~300 req/s with a simple ERB cache
Possible causes:

The Router uses a regexp match for even when it is not needed
Tilt can have a better template caching system than the one I am implementing
Rack::Builder

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2016

@mperham Feel free to add this to the examples section or update the wiki page when you want https://gist.github.com/badosu/5a6bb42cc0c0a7ae6e9b01313c61a5b9

@badosu badosu force-pushed the badosu-remove-sinatra branch 2 times, most recently from 0faf8b6 to 29b3577 Aug 2, 2016

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2016

With the last change, sidekiq-statistic should work as well now

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2016

@mperham I am pretty confident on the status of this implementation.

Let me know if there are any pending issues.

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 2, 2016

I'm not aware of any issues. It'll take me a week or two to get everything tested as this is a major release so be patient.

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 18, 2016

To test this branch, put this in your Gemfile:

gem 'sidekiq', github: 'mperham/sidekiq', branch: 'badosu-remove-sinatra'
get '/stats/queues' do
queue_stats = Sidekiq::Stats::Queues.new
unless using? ::Rack::Session::Cookie
unless secret = Web.session_secret

This comment has been minimized.

Copy link
@badosu

badosu Aug 18, 2016

Author Contributor

@mperham I guess you wanted to comment here?

I am not sure what is exactly the use case, but if sharing the session is required I think this will not work at all, because since Rails 4 it uses it's own middleware.

Using Sidekiq::Web::use users can follow the approach used in these tutorials: https://robots.thoughtbot.com/how-to-share-a-session-between-sinatra-and-rails http://stderr.timfischbach.de/2013/09/14/rails-4-and-sinatra-session-sharing.html

However, they should have a way to prevent usage of the ::Rack::Session::Cookie middleware, that is not implemented today.

If just reusing the same session secret is the use case, then I guess this would be a more straightforward way to configure it, as it enables setting more options directly:

### condiftion if Rails
Sidekiq::Web.use Rack::Session::Cookie, secret: 'v3rys3cr31', host: 'nicehost.org'
###

We might use this other approach as well: http://ap4y.github.io/2013/11/03/integrating-multiple-ruby-web-applications.html

Let me know what are the use cases for this that I'll take a look at what we can do and how to best integrate with Rails.

This comment has been minimized.

Copy link
@mperham

mperham Aug 18, 2016

Owner

Thanks for those links, that's really helpful. mounting in config/routes.rb is the main use case I wanted to handle; sounds like this is automatically handled by Rails.

This comment has been minimized.

Copy link
@mperham

mperham Aug 18, 2016

Owner

Oh, maybe we shouldn't set up any session middleware at all by default. Rails is the 90% use case and should provide one to us. We can document that config.ru users need to set up a session handler and we could provide a simple method to do it for them, e.g. Sidekiq::Web.default_session_handler!(secret).

@johnathanludwig

This comment has been minimized.

Copy link

commented Aug 18, 2016

I'm finding two issues in my testing. This is mounted in a rails 5 application.

  1. Passing the redis pool doesn't seem to be working. We mount two web instances. In development each instance points at a different DB on redis. Using this example:

    mount Sidekiq::Pro::Web.with(redis_pool: ConnectionPool.new { Redis.new(url: 'redis://localhost:6379/1') }), at: "/sidekiq_primary", as: "/sidekiq_primary"
    mount Sidekiq::Pro::Web.with(redis_pool: ConnectionPool.new { Redis.new(url: 'redis://localhost:6379/2') }), at: "/sidekiq_secondary", as: "/sidekiq_secondary"

    When I go to sidekiq_primary or sidekiq_secondary, both show the redis instance is redis://localhost:6379/0 instead of 1 or 2.

  2. Chrome is caching the pages sometimes displaying an old response. I think this is because the ETag header is coming back the same even after making changes. In some cases the ETag is being refreshed though. Here are the steps I went through:

    1. Go to Queues tab.
    2. Queue job in one queue.
    3. Refresh page. Queue should show 1 job queued.
    4. Click on queue to view job.
    5. Delete job. Page should be refreshed and show job removed.
    6. Go back to queues. Should say 0 jobs.
    7. Delete queue. Page should be refreshed with queue removed.

    Performing those steps should result in at least one of those expectations being incorrect due to the caching.

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 18, 2016

The sharding issue is fixed on Pro master. I'll coordinate the gem releases to make sure this isn't a problem.

On Aug 18, 2016, at 12:15, Johnathan Ludwig notifications@github.com wrote:

I'm finding two issues in my testing.

Passing the redis pool doesn't seem to be working. We mount two web instances. In development each instance points at a different DB on redis. Using this example:

mount Sidekiq::Pro::Web.with(redis_pool: ConnectionPool.new { Redis.new(url: 'redis://localhost:6379/1') }), at: "/sidekiq_primary", as: "/sidekiq_primary"
mount Sidekiq::Pro::Web.with(redis_pool: ConnectionPool.new { Redis.new(url: 'redis://localhost:6379/2') }), at: "/sidekiq_secondary", as: "/sidekiq_secondary"
When I go to sidekiq_primary or sidekiq_secondary, both show the redis instance is redis://localhost:6379/0 instead of 1 or 2.

Chrome is caching the pages sometimes displaying an old response. I think this is because the ETag header is coming back the same even after making changes. In some cases the ETag is being refreshed though. Here are the steps I went through:

Go to Queues tab.
Queue job in one queue.
Refresh page. Queue should show 1 job queued.
Click on queue to view job.
Delete job. Page should be refreshed and show job removed.
Go back to queues. Should say 0 jobs.
Delete queue. Page should be refreshed with queue removed.
Performing those steps resulted should result in at least one of those expectations being incorrect due to the caching.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 20, 2016

@badosu Any comments on (2) above?

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 20, 2016

We shouldn't be loading any ETag middleware at all. I don't really see much benefit for this specific webapp.

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2016

@mperham If you're asking about the ETag issue, I don't know how it can happen if we control all headers and we're not passing any ETag response headers at all.

Only if this is something being setup by Pro or a middleware customization.

On a side note, as soon as this PR lands in master I am gonna send a PR to to each plugin that relied on (now) unsupported Sinatra functionality.

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 22, 2016

Could this be part of the rails middleware stack when mounted in a rails app?

On Aug 22, 2016, at 08:35, Amadeus Folego notifications@github.com wrote:

@mperham If you're asking about the ETag issue, I don't know how it can happen if we control all headers and we're not passing any ETag response headers at all.

Only if this is something being setup by Pro or a middleware customization.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 24, 2016

Ok, gonna merge this as is and let it sit on master for a few weeks.

@mperham mperham merged commit 3f157e2 into master Aug 24, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mperham mperham deleted the badosu-remove-sinatra branch Aug 24, 2016

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 24, 2016

Thank you @badosu for your incredible work on this PR. It's a huge change and you've put an amazing amount of work into polishing it. I hope it proves stable!

@badosu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2016

@johnathanludwig Do you have any idea how we could reproduce this problem (the Etag one)?

@johnathanludwig

This comment has been minimized.

Copy link

commented Aug 26, 2016

@badosu These were the steps I ran. Along the way something would end up being cached by the browser and not display the expected results:

  1. Go to Queues tab.
  2. Queue one job.
  3. Refresh page. Queues should show 1 job queued.
  4. Click on queue to view job.
  5. Delete job. Page should be refreshed and show job removed.
  6. Go back to queues. Should say 0 jobs.
  7. Delete queue. Page should be refreshed with queue removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.