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

Đang mở
  • Có gì không ổn?
#835 0 bình luận 0 reaction 0 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 1 ngày

Chưa có ai nhận issue này.

Đánh giá

Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức phù hợp với người mới
45/100
Loại issue
Lỗi
Độ rõ ràng
Đặc tả rõ ràng
Mức độ hoạt động
Sôi nổi
Công nghệ
php
Lĩnh vực
backend, documentation, testing

Hướng nghiên cứu

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.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

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.
Ngôn ngữ chính
PHP
Star
1.7k
Fork
166
Merge trung bình
1 ngày 1 giờ
Pull request đã merge (30 ngày)
38

Chuẩn bị môi trường

Dự án này không cung cấp dev container, Dockerfile hay hướng dẫn đóng góp, nên bạn cần tự thiết lập môi trường: hãy bắt đầu từ README và xem hướng dẫn đóng góp lần đầu của chúng tôi để biết các bước chung.

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của goaop/framework

Tất cả issue của goaop/framework

Issue tương tự

Thêm issue về PHP

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.