Consider to remove exception handling from search_api_views_data

This has very good reasoning, this also makes the code a little bit less complex and removes indentation (that's great).

Hm, I don't know, removing error handling generally seems like a bad idea. On production site, often you'll just get a WSoD for uncaught exceptions, which is definitely also bad UX. And I don't know how common your problem is – I'd require more input from others for that. (And even then we won't actually know how many problems this would cause for others until we commit it and do a release including this.)
I still remember going through the whole D7 Search API ecosystem and adding exception handling. Would seem weird to remove it here.

Would it maybe also be an improvement if we only added the base table definitions once they're complete? (So, instead of doing $table = &$data[$key]; at the beginning of the loop body, do $data[$key] = $table; at the end.)

In any case, input from others would be very welcome here!

this also makes the code a little bit less complex and removes indentation (that's great).

While I agree with both of these points, I wouldn't make functional changes based on them.

PS: If the issue is caused by another contrib module, I'd be even less inclined to commit this.

@drunken monkey
I think we recently had this problem on one of our sites. Probably something went wrong while loading the Search API fields config, which totally messed up our search page, exposed filters were broken and also configured views filters (like current language) were broken. This caused the search page to show any search result in any language.

I would rather have 1 request WSoD or give an exception, which is probably fixed with a reload, than having a broken search page for hours.

I would rather have 1 request WSoD or give an exception, which is probably fixed with a reload, than having a broken search page for hours.

Put that way, it makes sense.
However, if this is not fixed with a simple reload, then you could potentially have a whole-site WSoD for hours, instead of just a broken search page.

Isn't that why SearchApiException is being caught? If anything from Search API breaks, it would still continue.
If anything else returns an Exception, something is probably very wrong with your website.

If something is very wrong with your website, wrong Search API Views table definitions are the least of your worries, and would most likely be automatically fixed in the course of dealing with whatever other problems there are.
Anyways, I do understand your point, for both of you, and it's duly noted. However, this is a complex problem, with the potential to cause a lot of frustration for others (and a lot of complaints for me), so I really can't do this with just one or two people speaking in favor of it. I'd either need a broader consensus that this is the right way to deal with this, or a veritable Views expert saying this is the right way.

Also, I'm still waiting for a reply to this question:

Would it maybe also be an improvement if we only added the base table definitions once they're complete? (So, instead of doing $table = &$data[$key]; at the beginning of the loop body, do $data[$key] = $table; at the end.)

Might this be a temporary improvement with less potential negative impact for the meantime?

I don't think it would help in my case. I believe the origin of my problem was a deadlock on the database.

I think we can safely say that this issue needs work. Either in the sense of convincing the maintainer of the need for this approach or in the sense of coming up with a better patch.

After debugging one of our live sites. I came to the same conclusion. The exception handling is leading to broken search api handlers when we receive a deadlock on cache_config. The result is that the search block + page is displayed, and all of a sudden it is gone, till you clear the cache again.

> Hm, I don't know, removing error handling generally seems like a bad idea.

I don't see what error is handled here. There's no attempt to determine the cause of the exception and for example restart the failed transaction. I doubt hook_views_data has enough knowledge here to even attempt such a thing.

The root cause on one of our sites happend to be a module that used config to store state, causing a race / deadlock on cache_config that the request rebuilding the views cache lost.

Hello,

I get the same errors in logs, mentionned errors on this post : https://www.drupal.org/project/search_api/issues/3029653. I get these errors since several days.... I don't know why I get these errors . How to resolve this issue?

Do you think that the problem come from a low ressource of the database server ?

Thanks.

Just chiming in to note that I'm also getting a deadlock inserting into cache_config when clearing cache. It doesn't seem to cause too much of a problem for the rest of the site, but does remove the ability for others without command line access to clear cache from the admin interface.

Please don’t chime in to say you get deadlock errors, that’s not the issue we’re discussing here. Chime in if the deadlock (or whatever other) errors occur(ed) inside search_api_views_data() and you want to vote for (or against) removing exception handling from that hook implementation.
Up to now, I count three to four votes in favor: webflo, jeroen.b, zuuperman and, possibly, Heine. That’s already a good start.

I'm in favor of removing the exception handling (per #16).

So, something like this?

@ Damien: Are you also in favor, or just wanted to help with an up-to-date patch?
Looks good, in any case, thanks!

Anyways, for everyone with the deadlock issue: Would in this case attempting to re-invoke the whole function, to do a second attempt, help?
If that still fails, we could then throw the exception, as that does seem to be the favored solution here. (We can also revert the patch again later in case it turns out it caused more problems than it solved.)
However, if restarting the function won’t ever bring a result anyways in this case (the deadlock), we might as well leave it out.

I don't really have opinion in the matter, I was just helping with the patch.

I had a view with a table of indexed fields.
If I had indexed nodes and change the view, drupal-site broke and the error shows:

[19-May-2021 13:41:18 UTC] Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 2006 MySQL server has gone away: INSERT INTO {cache_config} (cid, expire, created, tags, checksum, data, serialized) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6) ON DUPLICATE KEY UPDATE cid = VALUES(cid), expire = VALUES(expire), created = VALUES(created), tags = VALUES(tags), checksum = VALUES(checksum), data = VALUES(data), serialized = VALUES(serialized); Array
(
    [:db_insert_placeholder_0] => automated_cron.settings
    [:db_insert_placeholder_1] => -1
    [:db_insert_placeholder_2] => 1621431678.458
    [:db_insert_placeholder_3] => 
    [:db_insert_placeholder_4] => 0
    [:db_insert_placeholder_5] => b:0;
    [:db_insert_placeholder_6] => 1
)
 in /Applications/MAMP/htdocs/drushproject/mydrushproject2/web/core/lib/Drupal/Core/Config/CachedStorage.php on line 105 #0 /Applications/MAMP/htdocs/drushproject/mydrushproject2/web/core/lib/Drupal/Core/Database/Connection.php(665): Drupal\Core\Database\Connection->handleQueryException(Object(PDOException), 'INSERT INTO {ca...', Array, Array)
#1 /Applications/MAMP/htdocs/drushproject/mydrushproject2/web/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php(357): Drupal\Core\Database\Connection->query('INSERT INTO {ca...', Array, Array)
#2 /Applications/MAMP/htdocs/drushproject/mydrushproject2/web/core/lib/Drupal/Core/Database/Query/Upsert.php(106): Drupal\Core\Database\Driver\mysql\Connection->query('INSERT INTO {ca...', Array, Array)

Quick Fix

goto: /admin/config/search/search-api/index/YOURINDEX
click button: Clear all indexed data

Sorry, don’t really know what to make of that. It doesn’t really seem to be connected to this issue, just a different problem with an exception in Views. If the exception doesn’t occur when re-computing the Views data (that is, after clearing the cache, usually) then please create a new issue for your problem – with a bit more information, please.

I updated to search_api 8.x-1.20 and noticed the latest patch no longer applies cleanly.

I've re-rolled that patch. I don't have a lot of experience with patching but this did apply cleanly for me.

Fixed coding standard of #28

The fix (#29) does not work with multiple indexes. It only adds the search_api handlers for the last added index. The additional code should be inside the foreach loop.

I am experiencing the same issue as has been reported. This appears to have been triggered when the cache was flushed manually, which of course would call search_api_views_data().

Drupal\Core\Database\DatabaseExceptionWrapper while computing Views data for index Default: SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction: INSERT INTO "cache_config" ("cid", "expire", "created", "tags", "checksum", "data", "serialized") VALUES ...

The result was that the views exposed form block was hidden until another cache flush. It's likely the views page was also not working but I don't have any reports of that.

Ah, yes, seems the patch messed up the code flow a bit. Should be fixed with the attached.

Re-roll.

How's this patch working on Drupal 10?

Hello,

I am experiencing this as well.
What I don't really understand is why "not catching the exceptions" would prevent this from happening…?
From what I understand, this error happens because of simultaneous calls to search_api_views_data.
In my case as well, this happens when a cache rebuild is in progress while someone, at the same time is trying to access the page of an indexed entity. search_api_views_data seems to be triggered in both situations, leading to simultaneous inserts into cache_config table, leading to deadlock exception and incomplete definition into the cache system, that can be resolved with a new cache rebuild (provided there is no new conflict this time).
I don't understand the solution suggested to not catch exceptions… this will not prevent the deadlock from happening nor will it prevent the data from being incomplete in the cache system?
What do I miss…?

Personaly, I am happy to receive exceptions that let me know something bad happened.
What I would want though, is to understand how I could prevent this from happening.

I don't master yet what search_api_views_data does and when / how often it can be called…
I sometimes make a full cache rebuild on our production website without putting the whole site into maintenance mode. Until recently, I never had this type of error even with Search API in use. But this new deadlock problem with Search API is quite problematic until one can rebuild caches again so I'd like to understand what I should do or not do to prevent this.

Thank you!

The solution will not prevent your deadlock from happening. The solution will only make sure that your views data cache is not broken when the exception occurred.
By catching it, the caching system does not known that something went wrong and thinks the returned data is complete. While in realtime, it is missing all views plugins that still needed to be scanned.

The deadlock is not caused by views data itself, but just a combination of lot's of config, leading to a very big set of cache data that needs to be written during a full cache rebuild.

Patch is >2 years old now. Should someone turn #33 into a MR to push things forward? What do you think how to proceed here @drunken monkey?

Patch is >2 years old now. Should someone turn #33 into a MR to push things forward? What do you think how to proceed here @drunken monkey?

I’d still like feedback on my suggestion from #23: Would re-trying the whole function maybe help?
In any case, seems most people agree that, in the end, we should rather throw an exception than return incomplete Views data, so we can go with that when retrying failed (or right away, if people don’t think a retry is a good option).

Created an MR with a draft of what I’d have in mind. Would be great if a few people gave it a try (especially people whose site often encounters this problem) to see if just re-calling the function maybe fixes the problem at least some of the time?
Also, more general feedback on this problem and whether not catching the exception would be a good idea would still be welcome.

After digging a bit more:

It seems that Index::loadMultiple() in search_api_views_data() is responsible for writing into cache_config if the indexes are no longer in cache.

Also, it seems that search_api_views_data() (like all hook_views_data implementations) will only be called if both views_data:[language] and views_data:views:[language] entries are missing from table cache_default.

When the caches are being emptied, any visitor accessing a page will thus trigger search_api_views_data(), which will then try to write into cache_config because the indexes are missing from cache.

Drush cr will also call search_api_views_data() in its process.

Most probable scenario: drush cr gets here first, puts the lock on cache_config leading to the deadlock found error when search_api_views_data() gets executed by visitors requesting pages. This is no so bad since drush cr will eventually call search_api_views_data() anyway, so we could indeed not throw the exception.

Worst case scenario : a visitor triggers search_api_views_data() and puts a lock on cache_config preventing drush cr from rebuilding correctly the caches. This is bad and should be avoided.

Suggested solution:
I think we should only allow the execution of search_api_views_data() if triggered by drush cr or /admin/flush and stop its execution if triggered by a regular page request.

Here is the custom code I will be adding right now to my project in order to do so. What do you think?

<?php
if (php_sapi_name() === 'cli' && in_array('./vendor/bin/drush', $_SERVER['argv'])) {
  // case drush cr: do not block execution
}
elseif (\Drupal::routeMatch()->getRouteName() === 'admin_toolbar_tools.flush') {
  // case /admin/flush: do not block execution
}
else {
  // triggered by visitor requesting a page: stop execution to make sure no lock gets in the way of cache rebuilding
  return [];
}
?>

I would like to suggest one more thing: if Index::loadMultiple is indeed the code responsible for writing in cache_config when the indexes are not in cache (because search_api_index are configuration entities and not content ones…) Then I think maybe we'd better not use loadMultiple to retrieve index data inside search_api_views_data() which is a hook that will be called when caches are rebuilt. More direct database requests would avoid lock conflicts here on cache_config.

That being said, I still believe our best bet here is to prevent the execution of search_api_views_data() unless it is being triggered by drush cr or admin/flush because even if the lock conflicts are avoided for cache_config, there could be new lock conflicts when multiple simultaneous calls to search_api_views_data() try to write the same data at the same place.

For the record, I now understand better why we are having these lock conflicts on our production website when we thought rebuilding cache under maintenance mode would prevent them from happening. We have light ajax calls triggered on our pages every 20s to store our student progress. Normally these ajax calls never trigger search_api_views_data() even if all cache tables are emptied.
However, I noticed that when the site is under maintenance and all caches are emptied, calling one of these ajax urls will trigger search_api_views_data() this time. Normally, search_api_views_data() should be triggered only once... but if multiple ajax calls are requested quite simultaneously on a production website with lots of visitors, they will all trigger search_api_views_data and generate multiple deadlock errors in the logs. This is what we faced during our last deployment. The code I suggested above should protect us from that in the future I hope.

Suggested solution:
I think we should only allow the execution of search_api_views_data() if triggered by drush cr or /admin/flush and stop its execution if triggered by a regular page request.

If we do that, won’t Views just cache that empty array as the new Views data for the Search API, breaking all search views?

Also, I’m pretty sure other code is loading config entities during their hook_views_data() implementation as well, so if Views or Drupal cannot deal with that (reliably) I don’t think us working around that problem makes much sense – it should be fixed directly in Core. (At least as long as the workaround would be something as tricky as avoiding config entity loads.)

If we do that, won’t Views just cache that empty array as the new Views data for the Search API, breaking all search views?

The most important IMHO is to prevent search_api_views_data to put a lock on cache_config that would prevent drush cr from correctly rebuilding caches. Knowing that drush cr also calls search_api_views_data anyway, this is why I suggested this. But I have to confess that for my suggestion, I assumed that search_api_views_data would be called only when caches are being rebuilt. This is because when testing, the only way I could make a classic page request trigger search_api_views_data was when I emptied caches tables. If there is even a slight chance that hook_views_data are called outside of a cache rebuild situation, what you predict would indeed happen, the empty array could override the search data…
In that case, we should first try to detect if the caches are actually being rebuilt and only block the execution triggered by classic page requests when it is the case.

Also, I’m pretty sure other code is loading config entities during their hook_views_data() implementation as well

I had the exact same thought, but the other way around... I wondered: how come search API is the only one of our module to ever cause deadlock on cache_config in their hook_views_data implementation? So that's why I started to question the implementation and the use of Index::loadMultiple in that context.
I think this context is pretty tricky indeed. The fact that on a live website with many visitors, even with the site under maintenance hook_views_data can be called many times simultaneously is problematic.
More generally speaking though, I think maybe ideally the code responsible for rebuilding Search API data available for Views should not be triggered by visitors page call ever. Should all hook_views_data implementations never get triggered by visitors? Or should Search API rely on something else than this hook to rebuild its data for views...?

The most important IMHO is to prevent search_api_views_data to put a lock on cache_config that would prevent drush cr from correctly rebuilding caches.

I talked to @webflo today about this issue. We think that putting a lock, in addition to removing the return is probably a really good way to ensure that we always have a valid views defnition.

We never want to have a silently broken site.

Ok it seems there might be some confusion about my suggestion here.
I am not saying that search_api_views_data should not put a lock on cache_config when it needs to write inside that table.
My main point is that, when you know for sure that someone very important is about to write stuff really important inside a book, you don't want to take away the book and prevent them to do it just so that you can write down something they were gonna write anyway.
So, when we now that cache are being rebuilt, and that a lot of important stuff is about to be written in cache_config + the cache rebuilding process is about to call search_api_views_data anyway, we should not allow any other process to trigger search_api_views_data as well at the same time, thus blocking any writing inside cache_config, especially not a visitor requesting a simple page, especially while in maintenance mode. Which is what we are facing right now.

The problem here is not a silently broken site, it is a broken site that you cannot repair, because even in maintenance mode drush cr always fails because of the locks put on cache_config by search_api_views_data triggered by visitors requesting pages. The more visitors we have, the more risks we have of never being able to complete a drush cr on a production site.

So I agree that my code would need an additional condition that would make sure caches are being rebuilt before preventing any other processes than drush cr to go through simultaneously.

@borisson_: OK, so what would you (or @webflo) say is the best way forward here?

I have to be honest, I don't really remember all of that conversation, but I hope @webflo does.

Rebased.

MR 233 is patch #34

Attaching a patch made from MR !154 (second try approach) for applying via composer.json.

The problem here is not a silently broken site, it is a broken site that you cannot repair, because even in maintenance mode drush cr always fails because of the locks put on cache_config by search_api_views_data triggered by visitors requesting pages. The more visitors we have, the more risks we have of never being able to complete a drush cr on a production site.

Yes, we have had to work around exactly this in high traffic sites that silently failed, temporarily deleting the view and recreating it, e.g.

drush config:delete views.view.example_search_api_view -y
drush cache:rebuild
drush config:import -y
drush cache:rebuild

Which I guess is triggering the views_data rebuild

As a temporary code to discover and self-repair the issue via a cron job we have run this for our specific in case its helpful for others:

  /** @var \Drupal\Component\Plugin\PluginManagerInterface $views_row_manager */
  $views_row_manager = \Drupal::service('plugin.manager.views.row');
  $node_row_definition = $views_row_manager->getDefinition('entity:node', FALSE);
  if (!$node_row_definition || empty($node_row_definition['entity_type'])) {
      /** @var \Drupal\Core\Plugin\CachedDiscoveryClearerInterface $plugin_cache_clearer */
      $plugin_cache_clearer = \Drupal::service('plugin.cache_clearer');
      $plugin_cache_clearer->clearCachedDefinitions();
      /** @var \Drupal\Core\Cache\CacheTagsInvalidatorInterface $cache_tags_invalidator */
      $cache_tags_invalidator = \Drupal::service('cache_tags.invalidator');
      $cache_tags_invalidator->invalidateTags(['views_data']);
      /** @var \Drupal\Core\Cache\CacheBackendInterface $discovery_cache */
      $discovery_cache = \Drupal::service('cache.discovery');
      $discovery_cache->deleteAll();
  }

I am sure this was overkill, but at least gives a new chance for the issue to get fixed when it silently fails, in this case for a Search API View of Nodes.

In terms of moving this forward, would the 'second try' + log instead of exception MR be at least reducing the risk of silent failure for now? Happy to work on this if anyone can point me in the direction of how we might lock the config as outlined in #46.

I managed to reliably reproduce the error locally in ddev with a bit of a hack per https://www.drupal.org/project/search_api/issues/3029653#comment-16226130 and I can see that the current MR does not solve it. I provided details there rather than here as it could be this solves a separate issue.

We've been using a patch from MR 154 for many months now, and since then we did not experience the mentioned issue any more.
Patches from it didn't apply on latest version 8.x-1.40, so I rebased the branch.

Are there any others with good experiences using MR 154? @abramm?

The MR 154 has been working great for us since May.

Attaching the updated patch for applying via composer.json from MR 154 (second try approach), applies to Search API 1.40.

Thanks a lot for the additional feedback. Then I guess let’s finally go forward with this and see what happens.
Merged.
Thanks again for everyone’s work on this!