lock() currently still uses the lock_backends variable but we now have the lock service available.

lock() should be updated accordingly. We might have to move the service definition to drupal_container() for this to work, though..

CommentFileSizeAuthor
#38 lock-shutdown-function-1825450-38.patch7.26 KBBerdir
#34 lock-shutdown-function-1825450-34.patch7.26 KBBerdir
#34 lock-shutdown-function-1825450-34-interdiff.txt749 bytesBerdir
#33 lock-shutdown-function-1825450-33.patch7.16 KBBerdir
#33 lock-shutdown-function-1825450-33-interdiff.txt1.09 KBBerdir
#29 lock-shutdown-function-1825450-29.patch6.07 KBBerdir
#25 lock-shutdown-function-1825450-25.patch6.07 KBBerdir
#23 lock-shutdown-function-1825450-23.patch5.27 KBBerdir
#21 lock-shutdown-function-1825450-21.patch5.44 KBBerdir
#19 lock-shutdown-function-1825450-19.patch4.47 KBBerdir
#17 lock-terminator-1825450-17.patch11.08 KBBerdir
#15 lock-1825450-15.patch5.31 KBBerdir
#14 1825450-lock-debug-do-not-test.patch3.27 KBznerol
#14 1825450-lock-debug-system-test-reqtrace.txt273.89 KBznerol
#10 lock-1825450-6-rebase.patch5.31 KBznerol
#6 lock-1825450-6.patch5.32 KBBerdir
#6 lock-1825450-6-interdiff.txt501 bytesBerdir
#3 lock-1825450-3.patch4.83 KBBerdir
#1 lock-1825450-1.patch4.98 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
lock-1825450-1.patch4.98 KB

First, untested patch.

I think we should also move the lock documentation out of bootstrap.inc and to the lock interface, similar to how it's now done for cache API.

Status: Needs review » Needs work

The last submitted patch, lock-1825450-1.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
lock-1825450-3.patch4.83 KB

This should be better.

Status: Needs review » Needs work

The last submitted patch, lock-1825450-3.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

EntityFormTest failure looks unrelated, opened #1825568: Random test failure in Drupal\system\Tests\Entity\EntityFormTest for that.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
lock-1825450-6-interdiff.txt501 bytes
lock-1825450-6.patch5.32 KB

Added a __destruct() to call releaseAll() to the database backend.

I think we weren't doing this before to be able to use our shutdown wrapper that catches exceptions but I think we're already using __destruct() in other services.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

This is running for 1h20 now, I think I produced a loop.

Status: Needs review » Needs work

The last submitted patch, lock-1825450-6.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Ok, so it isn't a loop but it results in tons of apache segmentation faults. How the ... ?

znerol’s picture

znerol CreditAttribution: znerol commented
FileSize
lock-1825450-6-rebase.patch5.31 KB

Rebase onto current head.

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, lock-1825450-6-rebase.patch, failed testing.

znerol’s picture

znerol CreditAttribution: znerol commented

The good news: I got the following stack trace:

[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP Fatal error:  Access to undeclared static property: Drupal\\Core\\Database\\Database::$activeKey in /var/www/core/lib/Drupal/Core/Database/Database.php on line 158
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP Stack trace:
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP   1. {main}() /var/www/index.php:0
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP   2. Symfony\\Component\\HttpKernel\\Kernel->handle() /var/www/index.php:35
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP   3. Drupal\\Core\\HttpKernel->handle() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Kernel.php:193
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP   4. Symfony\\Component\\HttpKernel\\HttpKernel->handle() /var/www/core/lib/Drupal/Core/HttpKernel.php:51
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP   5. Symfony\\Component\\HttpKernel\\HttpKernel->handleRaw() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:73
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP   6. call_user_func_array() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:129
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP   7. Drupal\\Core\\EventSubscriber\\{closure}() /var/www/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php:0
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP   8. call_user_func_array() /var/www/core/lib/Drupal/Core/EventSubscriber/LegacyControllerSubscriber.php:52
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP   9. user_page() /var/www/core/lib/Drupal/Core/EventSubscriber/LegacyControllerSubscriber.php:0
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP  10. drupal_get_form() /var/www/core/modules/user/user.pages.inc:423
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP  11. drupal_build_form() /var/www/core/includes/form.inc:135
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP  12. drupal_process_form() /var/www/core/includes/form.inc:379
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP  13. drupal_redirect_form() /var/www/core/includes/form.inc:920
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP  14. drupal_goto() /var/www/core/includes/form.inc:1277
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP  15. drupal_exit() /var/www/core/includes/common.inc:710
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP  16. Drupal\\Core\\Lock\\DatabaseLockBackend->__destruct() /var/www/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php:0
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP  17. Drupal\\Core\\Lock\\DatabaseLockBackend->releaseAll() /var/www/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php:122
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP  18. db_delete() /var/www/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php:113
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP  19. Drupal\\Core\\Database\\Database::getConnection() /var/www/core/includes/database.inc:349

Always immediately followed by that one:

[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP Fatal error:  Cannot redeclare timer_start() (previously declared in /var/www/core/includes/bootstrap.inc:346) in /var/www/core/includes/bootstrap.inc on line 351
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP Stack trace:
[Tue Nov 06 19:58:29 2012] [error] [client 127.0.0.1] PHP   1. {main}() /var/www/index.php:0

The bad news: Even after commenting out $this->releaseAll(); in the destructor, the patch caused havoc all over the place. I can reproduce the segfaults and on the first glance it seems to me, that they are caused by a strange interaction between lock and cache.

znerol’s picture

znerol CreditAttribution: znerol commented
FileSize
1825450-lock-debug-system-test-reqtrace.txt273.89 KB
1825450-lock-debug-do-not-test.patch3.27 KB

Attached is output from apache error.log and a patch producing the messages in the log while running the system test group. I did not fully digest the data but it seems that the segmentation fault often is preceeded by something like lock release end: theme_registry:runtime:bartik:cache.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
lock-1825450-15.patch5.31 KB

Yes, __destruct()'s within the DIC seem to be evil. Let's try with that releaseAll() commented out, that did pass the book test locally.

Status: Needs review » Needs work

The last submitted patch, lock-1825450-15.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
lock-terminator-1825450-17.patch11.08 KB

Here is a patch that uses the terminator code from @katbailey. The lock test isn't going to work yet, because the test does an exit() and expects the lock to be released in a shutdown function, which doesn't work.

For reference, it does work fine when removing the exit and allowing the terminate event to run.

Two options that I can see:

- Make sure that the kernel is always terminated, e.g. by using a shutdown function.
- Use something else, at least for lock. A direct shutdown function for example, as we're doing it right now.

Status: Needs review » Needs work

The last submitted patch, lock-terminator-1825450-17.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
lock-shutdown-function-1825450-19.patch4.47 KB

Ok, as suggested by @catch, I've just moved the shutdown function into DatabaseLockBackend.

Status: Needs review » Needs work

The last submitted patch, lock-shutdown-function-1825450-19.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
lock-shutdown-function-1825450-21.patch5.44 KB

This should fix the DrupalUnitTestBase related failures, not sure about the others yet.

Nice to see how all the DUTB overrides are moving into the container slowly...

Status: Needs review » Needs work

The last submitted patch, lock-shutdown-function-1825450-21.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
lock-shutdown-function-1825450-23.patch5.27 KB

Ups, that was wrong, this should be better.

Status: Needs review » Needs work

The last submitted patch, lock-shutdown-function-1825450-23.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
lock-shutdown-function-1825450-25.patch6.07 KB

Ok, had to reset the statics before the new empty test container is created because ThemeRegistry needs the lock service to shut down. We need to convert that stuff to a service, similar to views data service, then this can be removed again.

The reason that this only happened in the simple test tests is that new theme functions where used that it tries to persist, which does not happen in CLI/manual test runs (although it can probably be reproduced there if you manually clear the cache first).

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

We seem to convert each procedural functions to just use the container, so I guess we should do that in a follow-up.

The patch looks really similar to #1880730-1: register a lock factory in the container though it actually works :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Fixed » Needs work

Er not quite, need to actually finish committing before commenting. This no longer applies.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
lock-shutdown-function-1825450-29.patch6.07 KB

Re-roll proudly presented by git rebase :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Let's get it back.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, lock-shutdown-function-1825450-29.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Strange, see #1874562-36: Upgrade path broken and yet tests pass

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
lock-shutdown-function-1825450-33-interdiff.txt1.09 KB
lock-shutdown-function-1825450-33.patch7.16 KB

Ok, figured it out, was quite logical after all.

The flag site check was inverted but due to lucky coincidences, this never blew up in the past only now that we need the lock service during variable_initialize(). Fixing that resulted in the mentioned notice.

Between setting the upgraded flag and rebuilding the system data, we never did a web requested, so never called refreshVariables() automatically. So global $conf was completely empty when rebuilding module data, which resulted in drupal_get_profile() returning the default value (standard), which added standard instead of minimal to the files list.

All I had to do fix that was to call refreshVariables() myself after enabling the flag...

This will hopefully pass.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
lock-shutdown-function-1825450-34-interdiff.txt749 bytes
lock-shutdown-function-1825450-34.patch7.26 KB

Small improvement to the comment so that the "No" does not imply that we have to negate the variable.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Thank you for that small last bit!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#34: lock-shutdown-function-1825450-34.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, lock-shutdown-function-1825450-34.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
lock-shutdown-function-1825450-38.patch7.26 KB

Why does this always conflict. We need an automated git rebase bot because there were no conflicts with that...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

So :)

We maybe should think about whether a direct shutdown function is still the way to go in Durpal 8, as it couples the lock code with some bootstrap.inc
What about following the path you started in earlier versions of the patch using symfony events? But sure this should be a follow up.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

That doesn't work for this, as terminate() is not called reliably, see #17 and #19.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Thanks! Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.