Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove support for preloading on Windows #4999

Closed
wants to merge 1 commit into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Dec 10, 2019

On Windows, the preloading functionality is fairly crippled, because classes that reference internal classes or constants in some way cannot be preloaded due to ASLR restrictions. As this is a very significant restriction, it will not be possible to preload any non-trivial codebase on Windows.

However, right now we still allow preloading as long as no internal classes are referenced -- this makes it seem to users like this is going to work, but sooner or later they will find out that it really doesn't.

I think that it would be better to disable the preloading functionality on Windows entirely to avoid unnecessary confusion.

@krakjoe
Copy link
Member

krakjoe commented Dec 10, 2019

This would appear to be the most reasonable course of action.

@andreasschroth
Copy link

This will help saving a lot of developers from wasting their time trying to get it run under Windows just to find out it's impossible.

@cmb69
Copy link
Contributor

cmb69 commented Dec 10, 2019

While preloading in its current state doesn't make much sense on Windows, and may be confusing for cross platform development, I suggest we should wait a while before removing support at all. Perhaps someone comes up with a reasonable fix before PHP 7.4.2RC1 will be tagged.

@nikic
Copy link
Member Author

nikic commented Dec 11, 2019

@cmb69 A possible alternative (longer-term) is to instead drop support for multi-process opcache on Windows, in which case we could avoid most of the other Windows issues as well. All of these issues do not exist in a threaded SAPI.

@nikic
Copy link
Member Author

nikic commented Dec 30, 2019

We should try to reach a decision here before the 7.4.2 release.

@cmb69
Copy link
Contributor

cmb69 commented Dec 30, 2019

Definitely! 7.4.2RC1 is supposed to be tagged on Jan 7.

@KalleZ
Copy link
Member

KalleZ commented Dec 31, 2019

I would rather we remove the broken implementation for Windows instead of supplying userland with a potential broken piece of software, as sad as it may be at least until we can maybe find a better solution that can work

@nikic
Copy link
Member Author

nikic commented Jan 6, 2020

As the consensus seems to be in favor of disabling preloading on Windows, I plan to merge this tomorrow before 7.4.2 is branched.

cc @derickr

Copy link
Contributor

@derickr derickr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brazenvoid
Copy link

@nikic Its unfortunate that this step had to be taken. We also would have to abandon all our work trying to make it work for our deployments. Unfortunately most of them are windows only due to corporate agreements.

Forgive my ignorance of applied protocols, but is there a RFC or logged task for migration to a threaded SAPI for opcache in the long term?

@cmb69
Copy link
Contributor

cmb69 commented Jan 23, 2020

@brazenvoid, no there is no RFC or logged task for that; however, it seems to me that this would be best done as new SAPI, and as such an RFC per se would not be a big task; the implementation would be though.

@nikic
Copy link
Member Author

nikic commented Jan 23, 2020

@cmb69 Aren't there already some threaded SAPIs for Windows? If yes, then the most direct way forward would probably be to allow preloading, but forbid process attachment if preloading is used. Possibly with a sanity check that the used SAPI is threaded (or CLI) in the first place, to get an earlier error.

@cmb69
Copy link
Contributor

cmb69 commented Jan 23, 2020

@nikic, this way, preloading support for Apache mod_php may be viable, but still not for FastCGI (i.e. IIS and NginX), but the former lacks impersonation support.

@bredmor
Copy link

bredmor commented Jan 23, 2020

Hey all, just discovered this topic and thought I might chime in with some potentially (or not) useful information.

I'm not a Windows guy, but I was helping someone with an issue involving an unrelated tool that wouldn't run under ASLR restrictions a couple weeks ago and we discovered that if you compile binaries on Windows 7 (or earlier) without explicitly setting the DYNAMICBASE flag, they will run without ASLR on Windows 10 or later, avoiding the normal system-wide enforcement that you can't turn off.

We didn't do any testing for whether or not this legacy functionality exists on Windows Server versions, and I don't know how useful knowing this information actually is, but it might be worth checking out if support for preloading on Windows is that highly desired.

@garygreen
Copy link

garygreen commented Aug 29, 2022

This will help saving a lot of developers from wasting their time trying to get it run under Windows just to find out it's impossible.

We use linux on server, but I was just curious to see how preloading performs and wanted to test it locally first - I suspect many other developers would try it too. Spent far too long trying to configure it and wonder why it wasn't working on Windows.

Can we get some kind of error log if the preloading was configured but php refused to use it? Maybe in the opcache.error_log file.

@nikic

@cmb69
Copy link
Contributor

cmb69 commented Aug 30, 2022

Can we get some kind of error log if the preloading was configured but php refused to use it? Maybe in the opcache.error_log file.

That should already be the case; an error "Preloading is not supported on Windows" is supposed to be logged to opcache.error_log.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants