Housekeeping/Upgrade: Puma #641

brunoocasali · 2020-10-07T14:51:25Z

Context

Puma released a new stable version (5.0) and in the company I work for we already using it, and we got some nice improvements. For this PR I've upgraded to version 5.0 and added clustered mode configuration (which removes latency/request queuing times)

Related: https://www.speedshop.co/2017/10/12/appserver.html

Summary of Changes

Upgrade puma gem to 5.x
Added clustered mode to serve more requests and reduce latency

Checklist

Tested Mobile Responsiveness
Added Unit Tests
CI Passes
Deploys to Heroku on test Correctly (Maintainers will handle)
Added Documentation (Service and Code when required)

DeeDeeG · 2020-10-08T16:35:34Z

Hi,

Thanks for the contributions, and happy Hacktoberfest, in case you're participating in that. 🎃

I have been taking a look at this. The puma upgrade definitely seems like a good idea.

Do you know much about the multiple workers, in terms of: Do we need any special consideration of thread safety, is parallelism safe/automatic, or are we meant to do some diligence to ensure our code behaves properly with more workers?

I'm ideally hoping to verify that this isn't a problem before merging. But it's a bit of a complicated subject. Any info you have on that is appreciated as I review this.

brunoocasali · 2020-10-13T19:52:32Z

Hi @DeeDeeG thanks for your reply!

Do we need any special consideration of thread safety,

Yes, when puma is used in clustered mode, we need to check all the sides of the app to look for thread-safe inconsistencies.
I ran a manual search over the source code but I was not able to find any signal of non-thread-safe code, I'll try an automatic search with rubocop-thread_safety it helped me when I migrated my production app in the company I work, (today is my deadline to give you an answer) (but any change in gemfile require a full rebuild in the docker setup, and my internet connection is really bad, that's why I pushed: #640 btw!!)

As I on the opensource side of the app, I really don't know if changing to clustered mode will improve the production app because I have no idea about the project workload. Ofcourse if you can share with me some data/insights we could improve this setup a lot to accomplish the best.

brunoocasali · 2020-10-13T21:34:57Z

@DeeDeeG finally!

As I mentioned earlier, I was trying to run this rubocop specs, and it does not show any offenses :)

root@fd1b0e28e7e7:/refugerestrooms# rubocop -r rubocop-thread_safety --only ThreadSafety,Style/GlobalVars,Style/ClassVars,Style/MutableConstant
Inspecting 121 files
.........................................................................................................................

121 files inspected, no offenses detected

So to me we are good to go with clustered mode activated!

DeeDeeG · 2020-10-26T21:16:17Z

config/puma.rb

@@ -21,7 +21,7 @@
 # Workers do not work on JRuby or Windows (both of which do not support
 # processes).
 #
-# workers ENV.fetch("WEB_CONCURRENCY") { 2 }
+workers ENV.fetch("WEB_CONCURRENCY") { 3 }


I'm 100% sure we want the Puma 5 update.

We will have to look at enabling multiple workers very closely to avoid breaking the site, or causing strange intermittent bugs. We have a fairly low (manageable) volume of website traffic much of the time, also.

I'd be inclined to merge this pull request without enabling multiple workers, and put it up on a staging branch with multiple workers enabled for about a month or two.

Not many person-hours per month are behind maintaining this web app at the moment. The web app also underpins the Android and (deprecated) iOS apps; The API server is part of the web app. So the whole of "Refuge Restrooms" stops working if our site stops working. And properly validating that this causes no bugs at all is a bit beyond my skills. Hence I am highly nervous/cautious/reserved with regard to enabling concurrency.

Ok, I understand the situation, so I'll update this PR to just update the puma, and I'll submit another with just the concurrency part, then we could deploy step by step :)

Thanks.

That would work very well.

I can merge the Puma update, and deploy that to the main site. Then activate multiple workers on the staging site for about a month or two. If all goes well, then the multiple workers change can be deployed to the main site as well.

@DeeDeeG done!

I've updated this PR and created the #654 hope we can merge it now :)

DeeDeeG

Approved just in case you forget to update this before Hacktoberfest ends.

I can handle merging this correctly if you forget. But Hacktoberfest ends soon! (In three days or so -- As long as it is October 31 in some time zone in the world, it is still Hacktoberfest.)

DeeDeeG · 2020-10-29T16:02:23Z

Thank you for following all the feedback and staying with this. I just merged this in.

Good to keep core packages such as Puma up to date! Enabling concurrency will take some more research and/or testing; #654 should cover that topic.

* Finish Upgrading Webpacker to v5 (#637) - Gemfile[.lock]: Update webpacker to 5.2.1 - Webpacker: Update config files Ran "rails webpacker:install" and committed the changes. In "config/webpacker.yml," kept the ".js.erb" entry under "extensions:" so that "app/javascript/packs/lib/maps.js.erb" is included. In "config/webpack/environment.js," kept jQuery and rails-erb-loader configs, as we are still using these. - JS dependencies: Upgrade webpack-dev-server Commit the changes from running `rails webpacker:install`, but using a caret "^" semver range for "@rails/webpacker", rather than an exact version. (So that we get updates in the 5.x series) * Update dependencies for September 2020 (#636) - dependencies: Remove `swagger` node module We don't actually use this. Also removes 200 sub-dependencies, and makes our yarn.lock file about 1270 lines shorter! - yarn.lock: Upgrade bootstrap to 4.5.2 (Was at version 4.4.1) - dependencies: Resolve node-fetch to ^2.6.1 - yarn.lock: Upgrade "selfsigned" and "node-forge" - Gemfile[.lock]: Update Rails and dependencies rails was 5.2.4.3, now it's version 5.2.4.4 * Keeping filters state on pagination (#638) - Keeping filters state on pagination - Fix Code Climates complaints. - Fixed active issue on ada filter buttons - Fixed undefined function problem when run rspec. * Add rubocop and resolve lint errors (#644) - Initialize rubocop - Run rubocop with --fix - Don't require magic comment This will affect every file and have potential side effects, so I'm going to start with this turned off. - Resolve lint errors in `app` - RSpec linting - Resolve remaining - Fix wrong change - Singleton method for verify - New lint rules - Add rubocop to travis config - Correct docker compose command - Check for geo The condition was actually supposed to be `if geo = results.first`, because that's not always obvious the intention and fails lint, I'm just doing a truthy check for it which should be the same. - Fix typo - Add contributing docs for rubocop * Applying ADA filters for the Map view of search. (#651) - Applying ADA filters for the Map view of search. - Changed comment for polyfill URLSearchParams - Fixed bug of map marker content not showing. * Add a standard EditorConfig configuration for every contributor to follow (#649) * Enhancement/i18n thread safe (#647) - Provide a way to respond requests without thread-safe issues As rails docs says https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests When we use 'I18n.locale =' the current locale could leak into following requests this is a standard and recommended way to deal with it :) - Create a shared_example to test I18n locale switching - Update spec/controllers/pages_controller_spec.rb * Upgrade puma to latest version 5.x (#641) * yarn.lock: Update "http-proxy" to v1.18.1 (fe195d7) Co-authored-by: Lucas <torres.giorgio@gmail.com> Co-authored-by: Tegan Rauh <3896310+tegandbiscuits@users.noreply.github.com> Co-authored-by: Bruno Casali <brunoocasali@gmail.com>

brunoocasali force-pushed the housekeeping/upgrade-puma branch 2 times, most recently from ac3d0e8 to 81bad55 Compare October 7, 2020 14:53

DeeDeeG added dependencies Pull requests that update a dependency file Hacktoberfest These are issues or pull requests related to Hacktoberfest (https://hacktoberfest.digitalocean.com/) packages Ready for Review labels Oct 8, 2020

brunoocasali mentioned this pull request Oct 14, 2020

Enhancement/i18n thread safe #647

Merged

4 tasks

DeeDeeG reviewed Oct 26, 2020

View reviewed changes

DeeDeeG previously approved these changes Oct 28, 2020

View reviewed changes

brunoocasali dismissed DeeDeeG’s stale review via 14563a2 October 29, 2020 12:55

brunoocasali force-pushed the housekeeping/upgrade-puma branch from fafce88 to 14563a2 Compare October 29, 2020 12:55

Upgrade puma to latest version 5.x

1617113

brunoocasali force-pushed the housekeeping/upgrade-puma branch from 14563a2 to 1617113 Compare October 29, 2020 12:56

brunoocasali mentioned this pull request Oct 29, 2020

Enable puma clustered #654

Merged

DeeDeeG merged commit f63d71f into RefugeRestrooms:develop Oct 29, 2020

brunoocasali deleted the housekeeping/upgrade-puma branch October 29, 2020 20:24

DeeDeeG mentioned this pull request Oct 31, 2020

Release 1.20.0 #655

Merged

5 tasks

DeeDeeG removed the Ready for Review label Nov 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Housekeeping/Upgrade: Puma #641

Housekeeping/Upgrade: Puma #641

brunoocasali commented Oct 7, 2020

DeeDeeG commented Oct 8, 2020

brunoocasali commented Oct 13, 2020

brunoocasali commented Oct 13, 2020

DeeDeeG Oct 26, 2020 •

edited

Loading

brunoocasali Oct 27, 2020

DeeDeeG Oct 28, 2020

brunoocasali Oct 29, 2020

DeeDeeG left a comment

DeeDeeG commented Oct 29, 2020

Housekeeping/Upgrade: Puma #641

Housekeeping/Upgrade: Puma #641

Conversation

brunoocasali commented Oct 7, 2020

Context

Summary of Changes

Checklist

DeeDeeG commented Oct 8, 2020

brunoocasali commented Oct 13, 2020

brunoocasali commented Oct 13, 2020

DeeDeeG Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

brunoocasali Oct 27, 2020

Choose a reason for hiding this comment

DeeDeeG Oct 28, 2020

Choose a reason for hiding this comment

brunoocasali Oct 29, 2020

Choose a reason for hiding this comment

DeeDeeG left a comment

Choose a reason for hiding this comment

DeeDeeG commented Oct 29, 2020

DeeDeeG Oct 26, 2020 •

edited

Loading