Advice order depends on cache warmth: aspects register in reverse order when their classes are autoloaded through the weaving pipeline

Abierto
  • ¿Algo no está bien?
#835 0 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
4/5
Tiempo estimado
3-5 días
Aptitud para principiantes
45/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
php
Área
backend, documentation, testing

Línea de trabajo

Start with Container registration order, then trace advisor handling through AdvisorIndex, CachedAspectLoader, AdviceMatcher, and the compiled caches under src/Core/Cache/. Add a functional test for tied advices on cold and warm caches, remove the production_native_aspects workaround in tests/Functional/CacheWarmupIntoAnotherDirectoryTest.php, and update the advice ordering docs. Done means builds are byte-identical across cache modes, tied advice runs in aspect registration order, and the requested test, analysis, and performance commands pass.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

Bug

Found while fixing #807 (PR #830).

Problem

Advices of the same kind and the same order value, coming from different aspects, are woven in a different order depending on how the aspect classes were loaded:

  • Cold build, and every debug build: the aspect classes are autoloaded through the stream filter, and the aspects register in reverse registration order.
  • Production kernel with a warm own cache: the aspect classes are loaded natively (skip list), and the aspects register in registration order.

Impact

  • Runtime behaviour: the interceptor lists of tied advices are swapped. Their runtime execution order differs between a cold build and a warm rebuild or a --cache-dir build, so the same release can run its advices in a different order depending on how its cache was produced.
  • Generated proxies differ between builds:
    • the alias of colliding imports moves to the other aspect;
    • the order of proxy methods changes;
    • the key order of _include.cache changes.
  • Builds are not reproducible: two builds of the same code are not byte-identical.

Reproduction (fixture project, production config, JIT off)

  1. rm -rf var/cache/aspect, then GO_AOP_CONFIGURATION=production bin/console cache:warmup:aop web/index.php. This warms the own cache.
  2. … --cache-dir=/tmp/warm
  3. rm -rf var/cache/aspect, then … --cache-dir=/tmp/cold
  4. diff -r -x _transformation.cache /tmp/cold /tmp/warm shows differences in:
    • Main.php
    • SimpleEnum.php
    • BackedEnum.php
    • ImportCollisionClass.php
    • _include.cache (key order; ignore the stamp line added by #816)

Debug builds, cold or warm, match the cold production build. Excluding src/Aspect/ from weaving makes all builds identical.

Cause chain

  1. On the first cache miss, SourceTransformingLoader::getTransformers() calls Container::getServicesByInterface(SourceTransformer::class).
  2. Its is_subclass_of($id, …) probe autoloads every pending factory id, and the aspects are pending factories.
  3. An aspect missing from the cache goes through AopComposerLoader, then the php://filter, then transformCode(), and back into getTransformers(). The calls nest once per aspect.
  4. At the innermost level, only the last-registered aspect is already loaded. AdvisorIndex::getAdvisors() (reached through ConstructorExecutionTransformer::$sourceMarkers) loads and registers it first, and each outer level then adds its own aspect.
  5. The advisors therefore end up in reverse registration order. AbstractJoinpoint::sortAdvices() keeps the advisor order for ties, so tied advices from different aspects come out swapped.
  6. With a warm own cache, the aspects are in the skip snapshot and load natively, so nothing nests and the order is the registration order.

A re-entry guard in getUnloadedAspects() alone does not help: the nesting enters through the container's probe for SourceTransformer.

Proposed decision for the implementing agent

Make the tie order explicit and independent of load timing (option c): sort advisors by the registration index of their aspect in the container, then by declaration order within one aspect, and only then let sortAdvices() apply the advice order.

  • Why: load order becomes irrelevant. This covers every way the nesting can arise, including future ones, and the order is something users can rely on and read in the docs.
  • Implementation sketch:
    • Container already keeps registrations in insertion order. Expose a stable registration index for a service id, preferably as an interface property or a small method on the internal contract, per the AGENTS rules.
    • AdvisorIndex/CachedAspectLoader record that index with each advisor.
    • AdviceMatcher (or the code that builds interceptor lists) orders by it before the advice-order sort. Closure advisors registered without an aspect sort after aspects, in their own registration order.
    • Check the compiled advisor caches (src/Core/Cache/): the stored order must be the deterministic one, so bump the format version if it changes.
  • Optional follow-up (option a), as a separate issue: stop Container::getServicesByInterface() from autoloading aspect ids while resolving SourceTransformer services, e.g. by resolving transformers by their registered tags or ids. This removes the nested weaving on cold builds, which is a performance and complexity win, but it is not needed for correctness once (c) is in.
  • Rejected: a re-entry guard only (option b). It handles today's path but leaves the order implicit.

Acceptance

  • A cold build, a warm rebuild and a --cache-dir build from a warm production cache are byte-identical, apart from _transformation.cache and the include-map stamp.
  • Tied advices from different aspects run in aspect registration order in every mode. Add a functional test with two same-kind, same-order advices from two aspects, checked on a cold and on a warm cache.
  • Remove the workaround in tests/Functional/CacheWarmupIntoAnotherDirectoryTest.php: drop the production_native_aspects fixture configuration and compare against the plain production configuration.
  • Document the tie order in the advice ordering docs.
  • composer test, composer analyze, and composer test:performance:transformation (the wiring changes). Run with opcache.jit=disable: the PHP 8.5.12 tracing-JIT bug breaks the functional subprocess tests.

Scoring

Score Why
Effort Medium A registration-index contract on the container, carried through the advisor index and matcher, a possible advisor-cache format bump, and functional cold/warm tests.
Impact Medium It only affects applications with tied advices across aspects, but there it changes runtime behaviour between deploys, silently.
Lenguaje dominante
PHP
Estrellas
1.7k
Forks
166
Merge medio
1 d 1 h
PR fusionados (30 d)
38

Preparar el entorno

Este proyecto no incluye contenedor de desarrollo, Dockerfile ni guía de contribución, así que la configuración corre por tu cuenta: empieza por su README y consulta nuestra guía para la primera contribución para los pasos generales.

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de goaop/framework

Todos los issues de goaop/framework

Issues similares

Más issues de PHP

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.