Hhvm: HHVM 3.6.0 ini_get not working

[sandeepone]

I'm unable to get ini_get('post_max_size') return empty string, which worked in 3.5.0.

echo ini_get('post_max_size');  // output ""
echo ini_get('max_post_size');  // output ""

echo ini_get('hhvm.server.max_post_size');  //output 104857600

Note: i'm not sure which one to use in HHVM so tried both, in Zend php it is 'post_max_size'
Thank you

[jeroenvermeulen]

This bug is probably what breaks image management in te backend of Magento on HHVM 3.6.0.
This is a serious problem.

[fcorriga]

even with ini_set('post_max_size', '25000');
ini_get('post_max_size') returns empty string

[fcorriga]

the problem occurs with upload_max_size too

[sandeepone]

I don't think we can set the post_max_size with ini_set, IMO it can only be set from ini file. Yes same issue with upload_max_size. A qucik ugly fix is use this ini_get('hhvm.server.max_post_size')

[fcorriga]

ini_get('hhvm.server.max_post_size') returns empty string too

[fcorriga]

well, actually it returns a boolean value (false)

[fredemmott]

cc @JoelMarcey

[rebeld]

FWIW, I'm only seeing this behaviour when vending through a webserver (nginx via fastcgi in my case). If I call ini_get through hhvn via the command line, it returns the values I would expect for 'upload_max_filesize' and 'post_max_size'.

[fcorriga]

exactly.

[BradStephens]

For post_max_size, use ini_get('hhvm.server.max_post_size').
For upload_max_filesize, use ini_get('hhvm.server.upload.upload_max_file_size').

Here's code for Magento users. app/code/core/Mage/Adminhtml/Block/Media/Uploader.php

public function getPostMaxSize()
{
    $post_max_size = ini_get('post_max_size');
    return $post_max_size ? $post_max_size : ini_get('hhvm.server.max_post_size');
}

public function getUploadMaxSize()
{
    $upload_max_filesize = ini_get('upload_max_filesize');
    return $upload_max_filesize ? $upload_max_filesize : ini_get('hhvm.server.upload.upload_max_file_size');
}

[fcorriga]

Does not work for me.

P.S.: sorry, it works.

[sandeepone]

The code should work as i'm using slightly different variant is working for me. Since I custom built the HHVM 3.6.0 on Centos 7 its working, I don't why it's not working for others.

[rebeld]

Sandeep, interesting, you are saying that hhvm 3.6 built from source is not exhibiting this behaviour for you? I'll have to give that a try.

[sandeepone]

I mean some people saying ini_get('hhvm.server.max_post_size') this not working

[rebeld]

Ah, got it, thanks for the clarification.

[BradStephens]

They're probably trying post_max_size instead of max_post_size. The hhvm team reversed it from what PHP calls it.

[jeroenvermeulen]

Will ini_get('post_max_size') and ini_get('max_post_size') work in the next version of HHVM again?
A lot of existing upload scripts break under HHVM 3.6.0.

[sandeepone]

Hopefully a minor version will be released with the fixes

[craigcarnell]

Any updates on identifying the code change that broke this?

[danslo]

Looking into this.

[danslo]

After a ~5 hour long bisect, 4bfeda9418331e6fea0de66a106b9ba5d04728a9 is the culprit.

/cc @rrh

[rrh]

Sorry for the issues here. I'll look into them on Mar 17.

[JoelMarcey]

My initial hunch is that it is having to do with settings that are set as PHP_INI_PERDIR...

[craigcarnell]

Any luck @rrh ?

[rrh]

I've just started looking at it now. Gotta wait for a debug build to finish. Then will wonder why there aren't any tests for this. Then will write some tests and look at the data flow through the PHP_INI_PERDIR

[danslo]

@rrh it's probably covered by a test, but it seems to happen in server / fastcgi mode and not on the command line. @JoelMarcey confirms that it's not specific to FastCGI though, so my initial hunch was threading, but PHP_INI_PERDIR sounds more likely.

In any case, hphp/test/server/fastcgi/tests will probably be a good place to place your new tests in.

[JoelMarcey]

@danslo @rrh I am quite convinced that it has to do with threading and how PHP_INI_PERDIR settings are handled in the new user callback map. I think we might be missing some sort of persistent store of the callbacks because when I run through gdb, the callbacks are empty. And that is because we are on a different thread (since the callback map is thread local)....

something like that :)

[danslo]

Yes, that's pretty much what I found as well, but I'm not sure why that diff would break it on first glance.

[JoelMarcey]

@danslo Yep. You provided me the hint :)

[rrh]

The logic here bears scrutiny. The encoding of Mode values smells, but I didn't want to change too much in the problematic commit.

I'm still trying to get a reproducer up and running, balancing this and meetings.

[rrh]

Here == https://github.com/facebook/hhvm/commit/4bfeda9418331e6fea0de66a106b9ba5d04728a9#diff-b5ba1b82c91a5eff79195e9db088e44aR722

[rrh]

Some breadcrumbs. Consider HPHP::get_callback https://github.com/facebook/hhvm/blob/master/hphp/runtime/base/ini-setting.cpp#L847

In CLI mode when presented with "post_max_size", the iter hits in s_user_callbacks.

In CGI/FCGI mode when presented with "post_max_size", the iter missed both s_system_ini_callbacks and s_user_callbacks, and so get_callback returns nullptr

[JoelMarcey]

@rrh Right. That's what I found too last night. When we switch threads, it becomes nullptr -- I suppose expectedly maybe?

[rrh]

In CGI mode, at master/head, HPHP::get_callback is searching for "post_max_size" in the thread-local CallbackMap s_user_callbacks. The search misses unexpectedly.

Analyzing printfs I put into the code, it turns out that the only thing IniSetting::Bind has previously bound to that CallbackMap are contributions starting from RequestInjectionData::threadInit, then contributions from various extensions, ending with contributions from StandardExtension::threadInitMisc. Needless to say "post_max_size" isn't in those contributions.

Should all INI_PER_DIR ini settings be added into the map at threadInit time? I don't fully understand what would happen if that were done, never mind the maintenance effort needed.

[danslo]

Why _are_ the ini settings bound in CGI mode without the offending diff?

[JoelMarcey]

@danslo: I think because they were just (possibly erroneously) placed in this non-thread local s_system_settings map and found there.

[rrh]

A sampling of PHP_INI_PERDIR from RuntimeOption::Load shows that these ini settings are in the same boat:
auto_prepend_file
auto_append_file
post_max_size
always_populate_raw_post_data
upload_max_filesize

[rrh]

I have an idea on how to refactor to make these PHP_INI_PERDIR settings to appear in the request ini settings. Right now I'm trying to revitalize the ini settings tests, which are at present emasculated, so that they can be (1) run and (2) run in fcgi mode. I want to show that my refactor fixes the problem, now and next week.

[dhpowrhost]

Has anyone found a workaround for this bug?

[rrh]

If you are building HHVM yourself, this PR might help: https://github.com/facebook/hhvm/pull/5057

[MarkGavalda]

Still no official solution for this issue, please? @rrh

[JoelMarcey]

@MarkGavalda as you probably know, @rrh has a pull request for this. There are some concerns though about the change, I think from a thread local perspective (you can see the comments on https://reviews.facebook.net/D35643). I suggested a conversation on #hhvm-dev about this.

[fcorriga]

FYI: hhvm 3.7 does NOT fix the issue.

[MarkGavalda]

@JoelMarcey thanks for the update. This issue makes HHVM break a lot of WordPress plugins unfortunately. :-/

[danslo]

It causes breakage for some Magento core behavior too. Given that this has been open for so long, maybe a revert is in order? I currently do not have the time to look into it.

[JoelMarcey]

@MarkGavalda I am back to working on ini after a month hiatus. While not directly related to this issue, I landed a big ini diff last night and am continuing to restructure some current diffs that I have out. I will look into this as well.

@danslo What would we revert exactly? Let's not revert anything just yet :)

[danslo]

@JoelMarcey I was talking about the commit that introduced this bug (but admittedly also fixed something else), 4bfeda9418331e6fea0de66a106b9ba5d04728a9.

[fcorriga]

why markw65 rejected the RRH patch? It seems to work

[MarkGavalda]

@fcorriga because: https://reviews.facebook.net/D35643#9

[fcorriga]

yeah, i've read that. It seems to me there are some misunderstandings...

[JoelMarcey]

@fcorriga What do you think the misunderstandings are?

As I understand it, the issue is that something like upload_max_filesize is a static in runtime-option.cpp, so that it remains constant (global) even on a per request basis. I.e., they are not thread_local. So how can you have per dir settings for those, as currently designed?

They would need to be brought out as thread_local settings (I think), but that would require a bigger change than what is in the PR currently.

[fcorriga]

Thanks, now i understand the issue. I was just missing the point.

[Kazanir]

Whatever the debate around per-directory INI settings, it is very frustrating that a bug that seriously affects lots of PHP frameworks goes unfixed for 2 major versions, the first of which was an LTS. :(

[JoelMarcey]

@Kazanir I just posted this to #hhvm too...

Admittedly, this is a bit of a mess. We have this commit https://github.com/facebook/hhvm/commit/4bfeda9418331e6fea0de66a106b9ba5d04728a9 and this PR https://reviews.facebook.net/D35643. If we can get the PR landed somehow (correctly), this issue should be solved. But how to get there is unclear right now. Another option is to revert the commit, but then that would break other things. I am pretty sure thread local is our friend here, but how to achieve that with these currently global static settings without breaking stuff is the tricky part.

Let me try to chat with @rrh on IRC today to see what we can do.

[Larceniii]

Thanks for working on this guys, I'm trying weird stuff trying to flip back to fpm when clients try to upload files... Stuff I shouldn't be doing !! Other than that great work here on hhvm, keep it up guys!

[JoelMarcey]

We hashed some stuff around today and might have some direction to start fixing this. We will keep you posted.

cc: @rrh

[jnthnclrk]

+1!

[Larceniii]

+1!!

[seanmowz]

Is anyone also having issues in magento where the product images will randomly disappear on front end with HHVM 3.6? Turning off HHVM returns all the pictures.

[craigcarnell]

@JoelMarcey Any updates? Thanks

[JoelMarcey]

Hi @craigcarnell ... @rrh and I talked last week. He was planning to investigate after an online discussion we had. I believe he had some other work come up (his day job), but was planning to look this week.

[Larceniii]

^^ Thanks for the update

[hartman]

Wikimedia downstream bugs because of this:
https://phabricator.wikimedia.org/T97415
https://phabricator.wikimedia.org/T98933

[Larceniii]

What's the status on this issue now?

[JoelMarcey]

@rrh is coming up with an update to his diff as we speak.

[JoelMarcey]

Thanks @rrh for the pull request!

For anyone who is building HHVM from source, if you would like to patch @rrh's pull request into your HHVM code, build and test to see if the problem is solved, that would be a helpful additional data point while we review the pull request internally.

https://github.com/newrelic-forks/hhvm/commit/301342ca14dca616a6d1a460975f90fa20eb8f93.patch

[lhorace]

@JoelMarcey I am also experiencing this bug as well. If no one picks this up in a couple of days and test this patch. I will pick it up and test it on a live server. I would really really like to try HHVM in a production environment and possibly recommend HHVM to others. Obviously, this is a show stopper, however thanks for looking into this!

Cheers

[EMembrillas]

Any new? today the problem persists in 3.7.1

[paulbiss]

This has not landed yet when it does we will close #5400 and this task. The patch is under review and you're welcome to follow along on phabricator, though some of the discussion has been moved off list.

https://reviews.facebook.net/D38907

[lhorace]

If have time today. I going to trial run a free hosting service with HHVM, either with the patch from https://github.com/newrelic-forks/hhvm/commit/301342ca14dca616a6d1a460975f90fa20eb8f93.patch against facebook-hhvm git or from https://github.com/newrelic-forks/hhvm. Report back on my findings.

[JoelMarcey]

@rrh update the diff after some comments provided to him. I think he is off to do some other work this week and won't be able to devote a bunch of time to it this week. I am going to patch his diff internally, review it, add any necessary tests and hopefully have it landed sometime this week.

[craigcarnell]

@JoelMarcey Good stuff, it will be nice to no longer have to do workarounds for this issue.

[aldo-gutierrez-tierconnect-com]

I had to try workarounds to install a Cherry Based theme on WordPress. Cherry plugin "import sample data" validates the following fields: post_max_size, upload_max_filesize, max_input_time, max_execution_time. Good to know that the issue is been solved

[craigcarnell]

@JoelMarcey Did you manage to find the time for this?

[JoelMarcey]

@craigcarnell I just finished writing a diff to help us get closer to using ini internally late last night; so I have a //little// break. I will look at this today.

That said, it would be extremely helpful if you or someone could take the patch, build it against master and try running with it to see if solves your problem. We have no real test or data point internally for perdir type settings because, well, we don't use them yet. Any concocted test I make up for this patch is no match for a real world test.

[JoelMarcey]

I just spent quite a bit of today reviewing the diff, making a few changes and doing some gdb inspection on the provided tests, etc. The diff seems reasonable to me and I think will be the unblocker for this issue. I have put it out for review internally after my changes and we will see what happens.

Again, that said, any build that is made on the outside with the patch would be quite helpful.

[JoelMarcey]

You can use the same patch available in #5400, but make the two changes I mentioned here.

https://github.com/newrelic-forks/hhvm/commit/9f2570bc9d5ed3dee24b4c0603b3c78ae5ea65f0#diff-b3a2df093a5cd2ea9a8c96e2a3b2dfa5R107

[craigcarnell]

@JoelMarcey I would imagine if it works php would simply return the correct post_max_size from the ini file! :) When is this going to land? It's been going on for what seems forever.

[JoelMarcey]

@craigcarnell and all -- this diff is in review. I believe it is in it's final stage (or very close). Tests are passing, etc., but you never know. Much of the HHVM team is involved in a group effort until the end of this week and key reviewers will be able to look it starting this weekend and early next week.

That said, I will reiterate, in the meantime, that the diff is available to patch into your HHVM codebase to compile and try out. https://github.com/facebook/hhvm/pull/5400 It has been updated to reflect pretty much everything we have done internally with it as well.

Thanks for your patience on this!

[Larceniii]

I agree with @craigcarnell this has been one of the more crippling and lingering hhvm bugs. I'm personally not good at patching diffs and I run stacks of various software and with hhvm team's past track record I thought I was willing to suffer through because there's a diff that already fixes it, hhvm updates frequently, and this was serious enough to push out the fix. Well, I was just plain wrong in my assumptions about this issue, and it's made me shy away from hhvm for a while.

I mean let's face it, I could have recompiled but I thought, if the hhvm guys fix it the next day after I did all that, it was an inefficient use of my time. I wish I had known it was going to take so long to get put in the repo.

Keep going strong guys, hhvm is awesome! I do appreciate all your hard work.

[JoelMarcey]

@Larceniii Believe me, I want to get this fix out there too. That said, we do need to make sure the fix is correct and doesn't affect existing infrastructure (Facebook and otherwise) because the global versions of these settings are used in production. The worst thing to have happen is for us to land this, get everyone's hopes up and then find something unexpected where we have to revert the diff and we are back at square one again.

This diff does look promising; all of our existing php tests (and the new ones associated with the diff) are passing. I am continuing to run some internal tests as well.

I do understand that not everyone can patch a diff; I just wanted to make sure that people on this issue thread who do compile HHVM regularly know that it is an option and could provide further data points on the validity of the fix. But, of course, we are reviewing it regardless.

Thanks!

[danslo]

In my opinion the biggest problem here is that it was a regression that fixed something in the zend compat layer but broke things in wordpress, mediawiki, magento and probably a bunch of others. Then took 3 months and 2 releases without getting fixed.

I know @rrh and the HHVM team are really busy, but all this could've been avoided by a simple revert.

[JoelMarcey]

Yeah, in hindsight I would have handled this differently. Probably reverted the original diff and had those that needed it patch with it until this could have been resolved. My lesson learned.

Given where we are now, we have the diff to (hopefully) fix this. If we find out it doesn't then we can go the revert route, assuming any changes needed to make it right would require something beyond a simple fix (i.e., a big redesign).

[oliver-graetz]

@danslo What's even worse is the fact that 3.6 is supposed to be the new LTS version. We had to go back to 3.3 after the new one broke file uploads.

[JoelMarcey]

@oliver-graetz, assuming this fix works, we can backport/cherry-pick the fix into previous releases.

[oliver-graetz]

That's not necessary for us. What I wanted to say, is that we were using 3.5 but couldn't revert from 3.6 to 3.5 when we discovered the error because http://dl.hhvm.com/debian/ did not have that 3.5 release anymore. So we had to go back to 3.3, which hurt even more.

[Larceniii]

@JoelMarcey Thank you for understanding our position on this and taking away something positive from it. To me, that's a big win for hhvm.

@oliver-graetz I was in the same boat on the 3.5 > 3.3 release and it wasn't nearly as stable as 3.5 in my custom wordpress environment.

I tried building hhvm 3.5 from source but the builds were doing very strange things like growing in size to 100s of megs on disk. I probably made some noobie mistake compiling but I spent so many hours on it only to finally give up and go back to 3.3 + fpm

I mean all this is possible because hhvm exists and I'm pushing the boundaries like I never dreamed possible because of the advancements in php due to hhvm. It's really an amazing piece of work, and I can't wait to get back on board.

[JoelMarcey]

@Larceniii Thank you. I am really sorry about all this heartache.

All -- let me give you status of our current thinking. I want to keep everyone in the loop.

We are looking to hotfix the diff that enabled this issue to begin with. I am looking into that as we speak. I have an idea that could work.

https://github.com/facebook/hhvm/commit/4bfeda9418331e6fea0de66a106b9ba5d04728a9

We don't want to revert the diff because people are depending on the functionality. But we can possibly work around the diff to provide an option not to use its functionality so that we can have a fix for the 3.6 LTS (for the upcoming 3.6.2).

Then we can let https://reviews.facebook.net/D38907 bake in master for a while to make sure it is stable. If it is, then we can remove the hotfix and have these two diffs live harmoniously in an upcoming point release.

cc: @jwatzman

[JoelMarcey]

A diff just landed into master and will be backported into the 3.6 LTS and 3.7 branches by @jwatzman to //hopefully// help alleviate this issue.

Basically, until a proper fix is accepted and known to work (which is probably https://reviews.facebook.net/D38907 or some variation), you can specify a new ini option in your server.ini (or equivalent config file) to disable the functionality that the offending diff that caused this issue provides.

hhvm.enable_zend_ini_compat=false

[jwatzman]

(And the reason false isn't the default is so we don't break anyone relying on this behavior, particularly in an LTS. 3.9 should have a proper fix, but notably the upcoming 3.8 will not.)

[craigcarnell]

@jwatzman Updated to the nightly and tested on my end, seems to read through fine! :+1:

With hvm.enable_zend_ini_compat=false

What 3.7 version will it be included in? 3.7.3?

[JoelMarcey]

@craigcarnell Great to hear. It should be in 3.6.4 and 3.7.2 (I don't think there is a tag for that yet, but the 3.7 branch has been updated).

Again, everyone, sorry it took so long to get a fix out here. What I thought was the right thing, turned out to be too time consuming and, lesson learned, get a hotfix out as soon as possible.

[Kazanir]

Thanks guys; various OSS frameworks thank you. :)

[sandeepone]

Thanks guys

[oliver-graetz]

Thanks!

[MarkGavalda]

Woot! Thanks guys.

[jwatzman]

3.6.4 is out with the hhvm.enable_zend_ini_compat=false option, and I'm running final tests on 3.7.2 which will also include it. I expect to begin building 3.7.2 in a couple hours, but when it lands for you depends on distro :)

[jwatzman]

For those on 3.7, the 3.7.2 release is ready but the host for dl.hhvm.com has been having some issues today, will get them up as soon as they are resolved.

[jeroenvermeulen]

Great work guys!

[chromafunk]

I am on 3.7.2 and the magento backend still suffers from the same. The buttons won't show up.

[JoelMarcey]

@chromafunk I am a bit unclear. Are you running a magento server? Did you put this option in your server.ini file

hhvm.enable_zend_ini_compat=false

[pepijnblom]

Thank you so much, this has solved the problem for us (Magento CE 1.9.1.1 on HHVM 3.7.2 on Ubuntu 14.04.2 LTS) by adding the line to server.ini

[grimabe]

I'm under Magento 1.9.1.0 with HHVM 3.8.1 FastCGI and the buttons do not appear even with the

hhvm.enable_zend_ini_compat=false

added to /etc/hhvm/php.ini and /etc/hhvm/server.ini files

any idea @JoelMarcey ?

Thanks

[grimabe]

My bad, it was a flash issue. Indeed on safari it didn't worked because flash plugin was missing. But in chrome it did.

[fcorriga]

i suggest to upgrade to 3.9.1 LTS if you're using it in a production server

[ksaltik]

Merging js cause this error not hhvm. Just merge js in frontend. We solve backend product image upload issue this way

[JoelMarcey]

I think this can be closed now, right? If not, please re-open.