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..

Files: 
CommentFileSizeAuthor
#38 lock-shutdown-function-1825450-38.patch7.26 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,597 pass(es).
[ View ]
#34 lock-shutdown-function-1825450-34.patch7.26 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch lock-shutdown-function-1825450-34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 lock-shutdown-function-1825450-34-interdiff.txt749 bytesBerdir
#33 lock-shutdown-function-1825450-33.patch7.16 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,567 pass(es).
[ View ]
#33 lock-shutdown-function-1825450-33-interdiff.txt1.09 KBBerdir
#29 lock-shutdown-function-1825450-29.patch6.07 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,199 pass(es), 0 fail(s), and 20 exception(s).
[ View ]
#25 lock-shutdown-function-1825450-25.patch6.07 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,033 pass(es).
[ View ]
#23 lock-shutdown-function-1825450-23.patch5.27 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 50,011 pass(es), 24 fail(s), and 42 exception(s).
[ View ]
#21 lock-shutdown-function-1825450-21.patch5.44 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 48,421 pass(es), 75 fail(s), and 42 exception(s).
[ View ]
#19 lock-shutdown-function-1825450-19.patch4.47 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,742 pass(es), 24 fail(s), and 52 exception(s).
[ View ]
#17 lock-terminator-1825450-17.patch11.08 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 48,195 pass(es), 1 fail(s), and 3 exception(s).
[ View ]
#15 lock-1825450-15.patch5.31 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 47,977 pass(es), 16 fail(s), and 5 exception(s).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] 42,258 pass(es), 4,417 fail(s), and 1,124 exception(s).
[ View ]
#6 lock-1825450-6.patch5.32 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 42,085 pass(es), 3,177 fail(s), and 809 exception(s).
[ View ]
#6 lock-1825450-6-interdiff.txt501 bytesBerdir
#3 lock-1825450-3.patch4.83 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 46,319 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#1 lock-1825450-1.patch4.98 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/install.core.inc.
[ View ]

Comments

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new4.98 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/install.core.inc.
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new4.83 KB
FAILED: [[SimpleTest]]: [MySQL] 46,319 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

This should be better.

Status:Needs review» Needs work

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

Berdir’s picture

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new501 bytes
new5.32 KB
FAILED: [[SimpleTest]]: [MySQL] 42,085 pass(es), 3,177 fail(s), and 809 exception(s).
[ View ]

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

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

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

znerol’s picture

StatusFileSize
new5.31 KB
FAILED: [[SimpleTest]]: [MySQL] 42,258 pass(es), 4,417 fail(s), and 1,124 exception(s).
[ View ]

Rebase onto current head.

znerol’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

znerol’s picture

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

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

Status:Needs work» Needs review
StatusFileSize
new5.31 KB
FAILED: [[SimpleTest]]: [MySQL] 47,977 pass(es), 16 fail(s), and 5 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new11.08 KB
FAILED: [[SimpleTest]]: [MySQL] 48,195 pass(es), 1 fail(s), and 3 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new4.47 KB
FAILED: [[SimpleTest]]: [MySQL] 49,742 pass(es), 24 fail(s), and 52 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new5.44 KB
FAILED: [[SimpleTest]]: [MySQL] 48,421 pass(es), 75 fail(s), and 42 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new5.27 KB
FAILED: [[SimpleTest]]: [MySQL] 50,011 pass(es), 24 fail(s), and 42 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new6.07 KB
PASSED: [[SimpleTest]]: [MySQL] 50,033 pass(es).
[ View ]

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

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

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

catch’s picture

Status:Fixed» Needs work

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new6.07 KB
FAILED: [[SimpleTest]]: [MySQL] 49,199 pass(es), 0 fail(s), and 20 exception(s).
[ View ]

Re-roll proudly presented by git rebase :)

dawehner’s picture

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’s picture

Status:Needs work» Needs review
StatusFileSize
new1.09 KB
new7.16 KB
PASSED: [[SimpleTest]]: [MySQL] 50,567 pass(es).
[ View ]

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

StatusFileSize
new749 bytes
new7.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch lock-shutdown-function-1825450-34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Thank you for that small last bit!

catch’s picture

Status:Reviewed & tested by the community» Needs work

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

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new7.26 KB
PASSED: [[SimpleTest]]: [MySQL] 49,597 pass(es).
[ View ]

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

dawehner’s picture

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

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

catch’s picture

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.