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

BUG: Reflection data unavailable in Production runtime #3402

Open
1 task done
kitsunet opened this issue Oct 12, 2024 · 0 comments
Open
1 task done

BUG: Reflection data unavailable in Production runtime #3402

kitsunet opened this issue Oct 12, 2024 · 0 comments
Labels

Comments

@kitsunet
Copy link
Member

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

This is a complex issue with multiple layers and no easy fix.

The ReflectionService currently has a special case in initialize for Production context specifically that will load a limited subset of cached reflection data compared to the full compiletime set. This distinction is generally very sensible as the amount of data is huge and shouldn't really be used during runtime anyways. BUT there is no limitation to calling the respective methods using this data, thus you unexpectedly get different results in Development and Production on the same codebase when asking the ReflectionService for example for getClassesContainingMethodsAnnotatedWith.
This is dangerous as it can lead to errors that appear only after a production deployment.

Expected Behavior

Loading all reflection data on production is prohibitive due to the RAM usage it would incur, so alternative solutions have to be found. A workaround exists in the form of CompileStatic annotation/attribute, which will execute the annotated static method at compiletime and store the results of that call as hardcoded return value in the generated proxy class. This works and is great for performance, yet has issues in itself. First of all CompileStatic is inert in Development again, so you get different proxy classes between Production and Development environments. While no known issues exist around this and the Flow core uses CompileStatic since years, it might lead to hard to track down effects as well.
As these two (CompileStatic and the different reflection data in Production) work hand in hand any steps changing this need to be taken carefully.

It could be benefial for example to change \Neos\Flow\Reflection\ReflectionService::initialize to not be special for Production but generally divide between compile time and runtime data for all appliction contexts. This however would immediately lead to compile static failing in Development and Testing as the necessary reflection data (as mentioned above CompileStatic is inert outside of Production) is not available anymore so the runtime call executing the static method will not receive the necessary data anymore.

The tandem step would be to enable CompileStatic for Development and Testing. This should generally be fairly safe, the biggest consideration is managing regeneration of the proxies (a quick and safe approach could be to just regenerate any proxy with compile static in it on any change to reflection data). To help debugging compile static behavior and what happens inside the static method we could provide a command that runs in compiletime and executes a given (compile) static method, so you can xdebug and observe it in isolation.

This combination of 3 measures together would be one route to take to clean this up a bit and prevent surprises in Production.

Next up to add additional safety would be a split between compiletime reflection and runtime reflection analog to the object manager, such that the runtime reflectionservice doesn't offer any methods using data not available at runtime.
This would be a breaking change however as even if we do not rename the ReflectionService and keep that as runtime ReflectionService, any CompileStatic method would have to use the (to be introduced CompiletimeReflectionService instead).
An easier but dirtier solution would be to throw in the respective methods when the data is not available (aka you try to access compiletime data at runtime).

Altogether different route of thinking (that I fear might be a waste of time) is trying to find a clever solution to load the minimum data necessary (and maybe lazy) to enable the functionality, but I am not sure a good tradeoff can be found here.

Steps To Reproduce

No response

Environment

- Flow: any
- PHP: any

Anything else?

No response

@kitsunet kitsunet added the Bug label Oct 12, 2024
kitsunet added a commit to kitsunet/flow-development-collection that referenced this issue Feb 7, 2025
This change tackles some problems within the reflection service that
stem from historically increasing complexity due to various caching
mechanisms depending on application context and compile time status.

The aim was to cut down on this complexity, while ensuring that all
existing use-cases continue working as intended.

This ultimately also fixes issue neos#3402 by providing the same
reflection data across all possible contexts.

A few features and caches got deprecated with this change and
could be breaking in the rare case you used the freeze package
api in your code:

The entire concept of freezing a package is deprecated

What remains are the commands in the package controller, which are now
all no-ops and deprecated to be removed with 9.0. This is to ensure
deployment pipelines possibly calling freeze commands do not break
with the 8.4 update.

Additionally the single method `PackageManager::isPackageFrozen`
remains, while the rest was removed. None of the methods was ever api
and it seems unlikely that someone used them in user-land code.
`isPackageFrozen` however is at the very least used in Framework and
Neos code and therefore remains until 9.0, but will now return false
for every package.

Caches deprecated and unused

With the simplification two caches are no longer needed, both are
still declared so that possibly existing cache configuration in user
projects doesn't error, but both

`Flow_Reflection_Status`

and

`Flow_Reflection_CompiletimeData`

will no longer be used and any content can be removed.

The only reflection cache is now `Flow_Reflection_RuntimeData`, which
makes the name somewhat deceptive as it is also used in compile time.
To avoid backwards compatibility issues however it makes sense to keep
the name for the foreseeable future.

Quick performance comparisons suggest that especially the initial
compile from empty cache benefits from this change. Reflection updates
in Development context afterwards seem to be on par with the existing
code base.
kitsunet added a commit to kitsunet/flow-development-collection that referenced this issue Feb 7, 2025
This change tackles some problems within the reflection service that
stem from historically increasing complexity due to various caching
mechanisms depending on application context and compile time status.

The aim was to cut down on this complexity, while ensuring that all
existing use-cases continue working as intended.

This ultimately also fixes issue neos#3402 by providing the same
reflection data across all possible contexts.

A few features and caches got deprecated with this change and
could be breaking in the rare case you used the freeze package
api in your code:

The entire concept of freezing a package is deprecated

What remains are the commands in the package controller, which are now
all no-ops and deprecated to be removed with 9.0. This is to ensure
deployment pipelines possibly calling freeze commands do not break
with the 8.4 update.

Additionally the single method `PackageManager::isPackageFrozen`
remains, while the rest was removed. None of the methods was ever api
and it seems unlikely that someone used them in user-land code.
`isPackageFrozen` however is at the very least used in Framework and
Neos code and therefore remains until 9.0, but will now return false
for every package.

Caches deprecated and unused

With the simplification two caches are no longer needed, both are
still declared so that possibly existing cache configuration in user
projects doesn't error, but both

`Flow_Reflection_Status`

and

`Flow_Reflection_CompiletimeData`

will no longer be used and any content can be removed.

The only reflection cache is now `Flow_Reflection_RuntimeData`, which
makes the name somewhat deceptive as it is also used in compile time.
To avoid backwards compatibility issues however it makes sense to keep
the name for the foreseeable future.

Quick performance comparisons suggest that especially the initial
compile from empty cache benefits from this change. Reflection updates
in Development context afterwards seem to be on par with the existing
code base.
kitsunet added a commit to kitsunet/flow-development-collection that referenced this issue Feb 7, 2025
This change tackles some problems within the reflection service that
stem from historically increasing complexity due to various caching
mechanisms depending on application context and compile time status.

The aim was to cut down on this complexity, while ensuring that all
existing use-cases continue working as intended.

This ultimately also fixes issue neos#3402 by providing the same
reflection data across all possible contexts.

A few features and caches got deprecated with this change and
could be breaking in the rare case you used the freeze package
api in your code:

The entire concept of freezing a package is deprecated

What remains are the commands in the package controller, which are now
all no-ops and deprecated to be removed with 9.0. This is to ensure
deployment pipelines possibly calling freeze commands do not break
with the 8.4 update.

Additionally the single method `PackageManager::isPackageFrozen`
remains, while the rest was removed. None of the methods was ever api
and it seems unlikely that someone used them in user-land code.
`isPackageFrozen` however is at the very least used in Framework and
Neos code and therefore remains until 9.0, but will now return false
for every package.

Caches deprecated and unused

With the simplification two caches are no longer needed, both are
still declared so that possibly existing cache configuration in user
projects doesn't error, but both

`Flow_Reflection_Status`

and

`Flow_Reflection_CompiletimeData`

will no longer be used and any content can be removed.

The only reflection cache is now `Flow_Reflection_RuntimeData`, which
makes the name somewhat deceptive as it is also used in compile time.
To avoid backwards compatibility issues however it makes sense to keep
the name for the foreseeable future.

Quick performance comparisons suggest that especially the initial
compile from empty cache benefits from this change. Reflection updates
in Development context afterwards seem to be on par with the existing
code base.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant