Don't override WEB_CONCURRENCY if set #926
Conversation
|
The real problem you're encountering, @dirkkelly, is one of buildpack order - if your app is a Ruby app, then you need to set Any tasks you'd like to run using |
|
Also see #931 |
|
Hi @dzuelke, thanks for getting back to me and letting me know about the recent updates to the script. To confirm, the buildpacks are ordered as nodejs then ruby. When using the official buildpack though the environment variable is overridden within the environment. When I swap out the official buildpack with the official github repository I see the same behavior However when I use this branch which conditionally sets the I understand this branch is now out-of-date though, but I hope it clarifies the point that our usecase is we need WEB_CONCURRENCY not to be overridden during the node build process, and because we set it to 0 for puma purposes it will be by this script. It looks like the #931 branch still explicitly sets the It seems like this is expected behavior as Is there any recommended practice for bypassing this script in the build process to avoid the environment variable being overridden? Thanks for all your help with this! |
|
Okay, I see now. This is actually an issue in the Ruby buildpack, which doesn't use the quasi-standard Could you try the branch from heroku/heroku-buildpack-ruby#1188 please? |
|
(right now, that branch in the PR will only help you if you have |
The Node.js, PHP and Python buildpacks write their WEB_CONCURRENCY defaults logic to this file name. This ensures that the last buildpack to run defines the defaults an app will use. If each buildpack were to use its own filename, then whatever file is evaluated first by the Shell on startup would set an initial value, and following files would see the variable already set and likely skip their calculation, or apply their own lower/upper limit enforcement, as it cannot know whether the variable was set by a user, or by some other buildpack's startup logic (the Shell sources files using a glob pattern from .profile.d/ on startup, which on GNU Bash is in alnum order, see https://www.gnu.org/software/bash/manual/bash.html#Filename-Expansion) This change uses the same filename as other buildpacks, ensuring that e.g. if heroku/nodejs is used as the first and heroku/ruby as the second buildpack, the defaults set by this buildpack will apply. Right now, in this constellation, .profile.d/ruby.sh runs first, and sets a default value, then .profile.d/WEB_CONCURRENCY.sh runs later, and does nothing, as the defaults fit within Node.js's current logic - but if a user sets WEB_CONCURRENCY=0 manually, then Node.js' logic kicks in, and re-sets the value to 1, as Node.js doesn't allow a value of 0. Ruby's Puma however allows "single mode", and for that, the value needs to be 0: puma/puma#2393 Refs heroku/heroku-buildpack-nodejs#926
|
@dirkkelly heroku/heroku-buildpack-ruby#1188 is now fully implemented if you want to test that branch and help verify it fixes your case |
|
Thank you so much for all your help @dzuelke! 🎉 I can confirm that your ruby buildpack branch has resolved the issue on my test environment. I will close this PR and subscribe to heroku/heroku-buildpack-ruby#1188 |
* use WEB_CONCURRENCY.sh for WEB_CONCURRENCY default The Node.js, PHP and Python buildpacks write their WEB_CONCURRENCY defaults logic to this file name. This ensures that the last buildpack to run defines the defaults an app will use. If each buildpack were to use its own filename, then whatever file is evaluated first by the Shell on startup would set an initial value, and following files would see the variable already set and likely skip their calculation, or apply their own lower/upper limit enforcement, as it cannot know whether the variable was set by a user, or by some other buildpack's startup logic (the Shell sources files using a glob pattern from .profile.d/ on startup, which on GNU Bash is in alnum order, see https://www.gnu.org/software/bash/manual/bash.html#Filename-Expansion) This change uses the same filename as other buildpacks, ensuring that e.g. if heroku/nodejs is used as the first and heroku/ruby as the second buildpack, the defaults set by this buildpack will apply. Right now, in this constellation, .profile.d/ruby.sh runs first, and sets a default value, then .profile.d/WEB_CONCURRENCY.sh runs later, and does nothing, as the defaults fit within Node.js's current logic - but if a user sets WEB_CONCURRENCY=0 manually, then Node.js' logic kicks in, and re-sets the value to 1, as Node.js doesn't allow a value of 0. Ruby's Puma however allows "single mode", and for that, the value needs to be 0: puma/puma#2393 Refs heroku/heroku-buildpack-nodejs#926 * it "has a newline between it blocks" do * named args for add_to_profiled and move ternary * changelog and... changelogs Co-authored-by: Richard Schneeman <[email protected]>
We would like to take advantage of the Puma Memory Reduction available through single mode.
However the Heroku/NodeJS buildpack currently overrides
WEB_CONCURRENCYwhen determining the best configuration based on system memorry.This pull request updates the script to keep the existing default if it is set, I'm not certain this is the best approach but thought this is a good way to start the conversation.