Support priority for needs_destruction service tag

Created on 6 March 2019, almost 6 years ago
Updated 19 February 2023, almost 2 years ago

Problem/Motivation

RegisterServicesForDestructionPass::process does not support a priority attribute. Being unable to set certain services to be destroyed after others has caused the following issue: https://www.drupal.org/project/purge/issues/3008776 πŸ› Search API list tag does not get purged Needs review

The Purge Queue service's destruct method is called before Search API's, resulting in certain tags not getting added to Purge's queue for invalidation.

Proposed resolution

The fix is simple enough, just sort the list before adding the registerService method. We can't take the approach used by, for example, BreadcrumbManager and simply key them by their priority because "priority" is an optional key (otherwise all modules implementing this tag would need to update).

If this gets in, Purge can add priority key and fix the aforementioned bug.

Remaining tasks

User interface changes

N/a

API changes

@todo

Data model changes

N/a

Release notes snippet

@todo

✨ Feature request
Status

Needs work

Version

10.1 ✨

Component
OtherΒ  β†’

Last updated 22 minutes ago

Created by

πŸ‡§πŸ‡ͺBelgium Grayle

Live updates comments and jobs are added and updated live.
  • Needs change record

    A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request β†’ as a guide.

    Moving back to NW for issue summary update, change record, and test coverage

  • πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

    #25 signed of on not needing additional tests, the existing coverage should be enough

    Updated the issue summary a bit to match the proposed fix.

    Will ping a release manager about #25 on the remove vs deprecate. That also influences the CR so not adding that until we know how we want ot use it

  • πŸ‡³πŸ‡±Netherlands Lendude Amsterdam

    Per @catch on slack:

    I also consider it internal but I think we could do a bare minimum deprecation (no test coverage, just the @deprecated stuff) just in case someone is doing something weird somewhere.

  • πŸ‡©πŸ‡ͺGermany donquixote

    Re-roll for Drupal 10.

  • First commit to issue fork.
  • Merge request !6107Priority for services β†’ (Closed) created by bradjones1
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    Converted to MR.

  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    Still NW per #38

  • Pipeline finished with Failed
    11 months ago
    Total: 530s
    #75133
  • πŸ‡©πŸ‡ͺGermany donquixote

    What is being deprecated?
    Maybe I am missing something..

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    We should leave RegisterServicesForDestructionPass in place and tag it as @deprecated in 10.3.x for removal in 11.0.x.

  • Merge request !8127Refactor service destruction β†’ (Closed) created by bradjones1
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    bradjones1 β†’ changed the visibility of the branch 3038040-support-priority-for to hidden.

  • Pipeline finished with Failed
    7 months ago
    Total: 166s
    #176821
  • Pipeline finished with Canceled
    7 months ago
    Total: 78s
    #176824
  • Pipeline finished with Failed
    7 months ago
    Total: 190s
    #176826
  • Pipeline finished with Failed
    7 months ago
    Total: 191s
    #176832
  • Pipeline finished with Canceled
    7 months ago
    Total: 43s
    #176836
  • Pipeline finished with Failed
    7 months ago
    Total: 191s
    #176837
  • Status changed to Needs review 7 months ago
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    Refactored this to use a service collector. Per #38 deprecated the subscriber however we'll still call any services that are added in any overridden implementations of the current subscriber. I think this is the right way to do this?

    Do we need tests for this? I think maybe not, since we'd just be duplicating testing of the tag collector...?

  • Pipeline finished with Failed
    7 months ago
    Total: 614s
    #176840
  • Status changed to Needs work 7 months ago
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    The test failures are... strange... but it's late. Will circle back tomorrow.

  • Pipeline finished with Failed
    7 months ago
    Total: 2457s
    #177572
  • First commit to issue fork.
  • Pipeline finished with Failed
    5 months ago
    Total: 570s
    #224800
  • πŸ‡ΊπŸ‡ΈUnited States bvoynick

    Merged in the latest 11.x branch. The same single test is still failing. These queries are not expected in core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php

    +    34 => 'INSERT INTO "semaphore" ("name", "value", "expire") VALUES ("active-trail:route:view.frontpage.page_1:route_parameters:a:2:{s:10:"display_id";s:6:"page_1";s:7:"view_id";s:9:"frontpage";}:Drupal\Core\Cache\CacheCollector", "LOCK_ID", "EXPIRE")',
    +    35 => 'DELETE FROM "semaphore"  WHERE ("name" = "active-trail:route:view.frontpage.page_1:route_parameters:a:2:{s:10:"display_id";s:6:"page_1";s:7:"view_id";s:9:"frontpage";}:Drupal\Core\Cache\CacheCollector") AND ("value" = "LOCK_ID")',

    The menu.active_trail service is marked with needs_destruction, so it makes some sense / appears to have some relation, but not that much. What appears to have changed is that the active trail service comes into use at all during this test. And I don't understand why this change to destructable services would change whether the active trail service is used - one would think this would have been happening before, and therefore the test failing before.

  • πŸ‡ΊπŸ‡ΈUnited States bvoynick

    I'm not able to apply a diff or patch for MR8127 to 10.3.x core, and #39 doesn't seem to apply on 10.3 anymore either. Here's a version of the current MR8127 made against 10.3.x specifically.

  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    Rebasing is preferential IMO to merging in from upstream, it helps keep the changes in the MR clearer. But, whatever, certainly not a hill to die on.

    I think the performance test query list is more about order of operations and may even reveal a lack of test coverage - I'm running into that a lot, lately. Not sure it's really a failure vs. an opportunity to update the test.

  • πŸ‡ΊπŸ‡¦Ukraine nginex

    Reroll for latest 10.3.x

  • πŸ‡ΊπŸ‡¦Ukraine nginex

    Ignore my previous comment with patch, #54 should work well (I just had a conflict with another issue)

  • First commit to issue fork.
  • Pipeline finished with Failed
    2 months ago
    Total: 572s
    #298788
  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Wouldn't be more easy to use the Symfony's PriorityTaggedServiceTrait::findAndSortTaggedServices() and just update RegisterServicesForDestructionPass instead of introducing a new service?

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Also, this needs some coverage.

  • Merge request !9853Alternative approach β†’ (Closed) created by claudiu.cristea
  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    I've added an alternative approach based on #59 (let's see the bot)

  • Pipeline finished with Canceled
    about 2 months ago
    #311645
  • Pipeline finished with Failed
    about 2 months ago
    #311650
  • Pipeline finished with Failed
    about 2 months ago
    Total: 118s
    #311659
  • Pipeline finished with Failed
    about 2 months ago
    Total: 791s
    #311666
  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    The approach from MR !9853 still needs tests. .Apart from that I see a consistent failure with StandardPerformanceTest which I could not relate to this change.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    MR!9853 looks like an interesting approach as it changes much less code. It has uncovered the same issue as noted in #53 where there are two extra database queries; it would be good to understand why this happens.

    The easiest way to add test coverage for destruction ordering will be to extend ServiceDestructionTest I think.

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    It has uncovered the same issue as noted in #53 where there are two extra database queries; it would be good to understand why this happens.

    I smell here an unrelated bug. Not long ago I have noticed a weird behavior with destructable services. As I remember it was a service not available anymore

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Here's my analysis

    How I've tested?

    I've applied this patch to DrupalKernel to record the sequence of "needs destruction" services:

    --- a/core/lib/Drupal/Core/DrupalKernel.php
    +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -687,9 +687,13 @@ public function terminate(Request $request, Response $response): void {
           // service was not used.
           foreach ($this->container->getParameter('kernel.destructable_services') as $id) {
             if ($this->container->initialized($id)) {
    +          file_put_contents('/tmp/dump.txt', "$id\n", FILE_APPEND);
               $service = $this->container->get($id);
               $service->destruct();
             }
    +        else {
    +          file_put_contents('/tmp/dump.txt', "$id --not-called\n", FILE_APPEND);
    +        }
           }
         }
       }
    

    Results

    11.x HEAD

    state
    module_handler
    theme.registry
    library.discovery
    block_content.uuid_lookup --not-called
    path_alias.whitelist --not-called
    path_alias.prefix_list
    Drupal\performance_test\PerformanceDataCollector
    drupal.proxy_original_service.menu.active_trail --not-called
    drupal.proxy_original_service.router.builder --not-called
    

    MR !9853

    state
    module_handler
    theme.registry
    library.discovery
    block_content.uuid_lookup --not-called
    path_alias.whitelist --not-called
    path_alias.prefix_list
    drupal.proxy_original_service.menu.active_trail
    drupal.proxy_original_service.router.builder --not-called
    Drupal\performance_test\PerformanceDataCollector
    

    It's easy to observe the issue. In 11.x HEAD, Drupal\performance_test\PerformanceDataCollector service runs before drupal.proxy_original_service.menu.active_trail, so it fails to collect queries ran by the later. Weird here (or maybe not?) is that drupal.proxy_original_service.menu.active_trail is not called at all because is not initialized (see the code from DrupalKernel).

    With MR !9853, the priority of PerformanceDataCollector (which is -1000) is honoured, so the collector allows all other services to run first and, voilΓ !, we have 2 additional queries which are related to active trail.

    I'm changing this to be a Bug because we see services relying on priority (the performance collector) but they fail to run at the end, as expected.

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    But a question remains: Why drupal.proxy_original_service.menu.active_trail was not called at all with 11.x HEAD?

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Added missed queries. Also, because menu.active_trail destruction failed to run the cache set/get counts were also not accurate.

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    This is not ready because it still lack test coverage. However, I'm setting to "Needs review" to get some feedback on analysis but also on the question from #67 ✨ Support priority for needs_destruction service tag Needs work

  • Pipeline finished with Failed
    about 2 months ago
    Total: 510s
    #312923
  • Pipeline finished with Success
    about 2 months ago
    Total: 1044s
    #313422
  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄
    • Test coverage was adde (removed tag).
    • Pipeline is green. I've also ran "test only" job to prove the bug.
    • Closed the other MR

    Ready for review.

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Did a global search for projects defining destructable services with priority https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22needs... . It turns out that performance_test module is the only one doing it. I think it's correct to fill this as a bug.

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Hide patches.

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Updated IS

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Rebased after πŸ“Œ OOP hooks using attributes and event dispatcher Needs review

  • Pipeline finished with Success
    about 2 months ago
    Total: 352s
    #313974
  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Probably we can use PriorityTaggedServiceTrait::findAndSortTaggedServices() to simplify code in other DI classes. But that could be for a followup

  • Pipeline finished with Success
    about 2 months ago
    Total: 730s
    #314446
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    I didn't know about PriorityTaggedServiceTrait prior to this, but like how it is purpose-built for this. We just need to implement it. Very straightforward, I think this is RTBC.

  • First commit to issue fork.
  • Pipeline finished with Success
    about 2 months ago
    Total: 642s
    #317322
    • catch β†’ committed 418c1db3 on 11.x
      Issue #3038040 by bradjones1, claudiu.cristea, swentel, bvoynick,...
  • πŸ‡¬πŸ‡§United Kingdom catch

    Committed/pushed to 11.x, thanks!

    This doesn't cherry-pick cleanly to 10.4.x. I think it is probably valid for that version, but also not the end of the world if we don't backport it. So marking fixed, but happy to backport if someone opens a backport MR.

  • Merge request !9916Drupal 10.4 version β†’ (Closed) created by claudiu.cristea
  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    Added a new 10.4 MR

    • catch β†’ committed 25ad53d3 on 10.4.x
      Issue #3038040 by claudiu.cristea, bradjones1, swentel, bvoynick,...
  • πŸ‡¬πŸ‡§United Kingdom catch

    Thanks for the quick turnaround on the backport.

    Committed/pushed to 10.4.x, thanks!

  • Pipeline finished with Success
    about 2 months ago
    Total: 727s
    #318114
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024