RefugeRestrooms / refugerestrooms

REFUGE restrooms indexes and maps safe restroom locations for trans, intersex, and gender nonconforming individuals.

http://www.refugerestrooms.org

GNU Affero General Public License v3.0

893 stars 261 forks source link

Housekeeping/Upgrade: Puma #641

Closed brunoocasali closed 3 years ago

brunoocasali commented 4 years ago

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)

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 commented 4 years ago

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 commented 4 years ago

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: https://github.com/RefugeRestrooms/refugerestrooms/pull/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 commented 4 years ago

@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 commented 3 years ago

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.