implement getenv and putenv in go #1086
Conversation
|
In theory, we could call the go-side to fill the track_vars array. However, I believe this would end up conflicting with #1080 and in any case, would be an optimization for after that is merged. |
|
huh. I guess I will have to do that since ASAN found go/php stepping on each other. |
|
I think I realized, why $_ENV sometimes behaves weird #1061 . We can fix that in one swoop or a separate PR after this one. Basically:
FrankenPHP does actually reload We should probably not destroy We could also fix |
|
Yes @AlliBalliBaba, I think after this PR, we are in a place were we can implement the env in a php-fpm compatible way (if it isn't already). We should do some testing on fpm + frankenphp and identify any differences between them. Your PR gets us most of the way there, but this one just unifies Go/PHP environment access. I think there is still more to be done that is out of this PR's scope. |
|
In other words, after this PR, the environment is entirely managed in go (whether it actually gets the environment or from a map), so we can do whatever we need to. There might be a slight performance penalty, due to the cgo call, but getting it right in a higher level language will probably be easier before we optimize back to c (if we even need to). |
|
Not actually sure how to test |
frankenphp.c
Outdated
| @@ -472,7 +563,28 @@ int frankenphp_update_server_context( | |||
| return SUCCESS; | |||
| } | |||
|
|
|||
| PHPAPI void get_full_env(zval *track_vars_array) { | |||
| struct go_getfullenv_return full_env = go_getfullenv(thread_index); | |||
There was a problem hiding this comment.
I think we can probably cache the environment somewhere here as a thread local (or even global) zend_array, so it will only get imported once. I'll check later this week again, but I think that's how FPM also behaves with since putenv doesn't alter $_ENV or $_SERVER
There was a problem hiding this comment.
Should probably do it on the go side and not mix source of truths, but yeah.
There was a problem hiding this comment.
Yeah even in the manual someone mentions how putenv doesn't affect $_ENV. Since $_ENV is unchanging for the duration of the process I think it would be most efficient to directly store it as a zend_array at the start of the process. I can also do that in a follow-up PR if you want.
putenv and getenv should always access the real environment though like in your implementation 👍
There was a problem hiding this comment.
This is working as it is supposed to. This only gets called in the following scenarios:
- on each request in cgi-mode when
$_SERVERis filled - at worker startup in worker mode to fill
$_SERVER getenv()without any parameters- jit-ENV when
$_ENVis first accessed
There shouldn't be any reason to cache this.
|
This should be good to merge! |
Inspired by the discussion in #1080, this is an ultra-risky implementation that should fix #915. Basically, Go uses RWLocks to prevent multiple threads from writing to the environment, while PHP uses TSRM. TSRM and Go don't know about each other and if one happens to be reading the environment while the other is writing to it, a segfault is bound to happen.
So... this PR replaces the built-in
getenvandputenvwith ones that defer to Go. See #1080 discussions for why this may not be the best solution, but maybe it is a 'good enough' solution to prevent segfaults from concurrent environmental accesses in Go and PHP. If we are happy with this approach, this will need many tests to ensure the behavior is exactly the same as the built-in ones.For example, a missing feature is setting the
TZenvironment variable which should trigger a reset of the current timezone... however, that would bleed into other threads currently running, so it isn't ideal. In fact, it might be good that it doesn't work.