Race condition detecting ENV variables

[zanderwar]

PRs:

• [x] https://github.com/silverstripe/silverstripe-framework/pull/7484
• [x] https://github.com/silverstripe/silverstripe-assets/pull/87
• [x] https://github.com/silverstripe/silverstripe-behat-extension/pull/170
• [x] https://github.com/silverstripe/silverstripe-serve/pull/19

Replicate:

1. Install SS4 (framework only or full doesn't matter happens on both)
2. Ensure you have your .env configured correctly and mod_env enabled
3. Navigate to the homepage
4. (Optional) Remove install.php
5. Spam refresh as fast as possible letting the page load each time.

Inevitable Error:

If you've removed install.php

SilverStripe Framework requires a $databaseConfig defined.

If you havn't removed it

Redirected to install.php

Affecting: Everyone

• Easy to replicate on WIN/Linux
• Yet to replicate on OSX

[zanderwar]

@colintucker also replicated this issue by spamming clicks in the site tree within the CMS to inevitably receive "Server Error" in the X-Status

[colintucker]

Can confirm I have encountered this issue in dev. As @zanderwar mentioned I can generate server errors by spamming site tree clicks as well.

[tractorcow]

See https://gist.github.com/progmars/1e545d96dd48676a2aa7

Issue is due to putenv acting async.

[tractorcow]

A better example https://github.com/laravel/framework/issues/7354

[tractorcow]

It looks as a result of this issue is that we'll need to replace getenv with another thread-safe equivalent. Will be another beta change at least. =(

[tractorcow]

phpdotenv is made for development environments, and generally should not be used in production. In production, the actual environment variables should be set so that there is no overhead of loading the .env file on each request. This can be achieved via an automated deployment process with tools like Vagrant, chef, or Puppet, or can be set manually with cloud hosts like Pagodabox and Heroku.

https://github.com/vlucas/phpdotenv#usage-notes

Should have paid more attention!

[sminnee]

Can we confirm that no other popular framework uses getenv? If that’s not the case then I’d dispute the diagnosis.

[zanderwar]

@sminnee I believe laravel canned it for their own methodology. I researched everywhere regarding this issue and recall that being the case somewhere. However there is hard evidence everywhere (and in #ss4 of slack this morning) proving that this is an issue for all operating systems (except OSX so far) - especially Windows.

If this is something that gets swept under the rug because other frameworks couldn't be bothered because other popular frameworks couldn't be bothered than I personally (among others) am stuck on lesser versions than 4.0 <3

[tractorcow]

It's not simply the usage of getenv(); If you set your environment vars on your platform (e.g. ssp), it's safe, but loading these in the same request obviously has a race condition. However, we give the impression that .env files are safe to use in production when they obviously aren't.

[sminnee]

Should we introduce an env() getter that works the same way as Laravel’s, and only gets defined if it doesn’t already exist?

[sminnee]

Or do we recommend that people avoid using .env in prod?

[zanderwar]

~SetEnv has also proven unreliable. At least from .htaccess~

I've settled for DB::setConfig() until this tides over into being reliable, in my scenario I don't need any additional env vars,

~Ergo, this issue remains even in prod & without .env~

[zanderwar]

Was a little quick on the refresh in this GIF but note that moment where the array is empty before sending me to install.php

Note: This GIF is from using .env

[sminnee]

If SetEnv is unreliable something seriously FUBAR is going on. Are you certain about that? Perhaps do a reinstall of apache?

[zanderwar]

Sorry, can confirm it's working in .htaccess

[sminnee]

If we use symfony/dotenv instead of vlucas/phpdotenv do we get any better results?

[sminnee]

It looks like Symfony has a wrapper around getenv for the same reason: https://github.com/nicolas-grekas/symfony/blob/f76e420e0959b52e73f1486a7b99771d6553ff01/src/Symfony/Component/DependencyInjection/Container.php#L436

Lesson: getenv() on its own is a bad API :-(

[dhensby]

Hmm - looks like we'll need to define our own getenv to mitigate this, then?

we knew that dotenv library wasn't meant for production use but at the same time we do have to accept that some people will use it in prod because of limitations outside of their control. We should aim to get it working as much as we can IMO whilst still saying it shouldn't be used on prod.

[NightJar]

Incoming PR.

Important questions first though, I have to do the hardest thing in programming: Will SilverStripe\Core\Environment::shimFromFile and SilverStripe\Core\Environment::env do?

I thought ::get was a bit... on the nose, leading to Confusion++ since there is alerady SilverStripe\Core\Environment::getVariables which... doesn't get environment variables :( (certainly caught me out when investigating :/ )

c.f. preliminary ideas at: https://github.com/silverstripe/silverstripe-framework/commit/e614edec61f396c7e8f29bd5ee3340f56cf0b1a6

[tractorcow]

I've done a bit of work after looking at @NightJar 's initial fix. Please check my PR at https://github.com/silverstripe/silverstripe-framework/pull/7484.

[NightJar]

Yeah, looks all good to me :)