Check Suspended Content Cache is bypassed in all of the 26 places that it is called -> Considerable duplicate queries

[nickchomey]

Describe the bug The check_suspended_content function gets and sets an object cache group, but it literally never gets used because in ALL of the 26 places that called, a "true" argument is passed for $force, which bypasses the cache. Actually, it not only bypasses it, but it gets and sets the object cache each time - only to never be used.

This results in a TREMENDOUS amount of duplicate queries throughout the website.

I do not see the need for it to be bypassed at all, given that the cache item is cleared by the bb_moderation_clear_suspend_cache function, which is hooked into any changes to suspensions by

add_action( 'bb_suspend_before_add_suspend', 'bb_moderation_clear_suspend_cache' ); add_action( 'bb_suspend_before_remove_suspend', 'bb_moderation_clear_suspend_cache' );

I suggest one of the following:

1. all of the "true" arguments be changed to "false"
2. remove the bypass altogether
3. or be more selective with which functions bypass the cache

To Reproduce Steps to reproduce the behavior:

This is just one example, but it surely happens in nearly every part of the site.

1. Install query monitor
2. Do a network search
3. Look through the duplicate queries for BB_Core_Suspend::check_suspended_content()
4. Change line 446 in bp-moderation\classes\suspend\class-bp-core-suspend.php from if ( false === $result || true === $force ) {

to

if ( false === $result || false === $force ) {

and you will see the duplicate queries disappear.

Expected behavior I would expect this cache object to actually be used.

Screenshots

Duplicate queries on a search

all of the instances in which the check_suspended_content function is called, all passing "true" to bypass the cache

Environment Which site did you replicate this issue on?

A local and webserver environment

Support ticket links

Jira issue : PROD-710

[nickchomey]

This issue appears to be broader than just check_suspended_content

check_hidden_content - more than half of the calls bypass the cache

check_moderation_exist

It is peculiar that only some of the checks have a bypass - the following don't have a bypass

• check_user_suspend
• check_suspend_details_exist (doesnt even have a cache - probably should?)
• check_blocked_content (doesnt even have a cache - probably should?)

as well as plenty of other check functions in other files.

[nickchomey]

This appears to be the commit where the bypass caching was added. I really don't understand the rationale behind all of them being set to true. Surely it was just a mistake? If not, then there needs to be a better mechanism for clearing/updating the cache, rather than not using it at all.

https://github.com/buddyboss/buddyboss-platform/commit/7556ddadf79fabbea259616e4633ffad29156f20

[gregburg]

I've found 945 duplicates in query logs just for one user action - loading 20 more activity feed posts

[nickchomey]

@gregburg can I ask how you generated that log/where you found it?

[gregburg]

mysql> SHOW VARIABLES LIKE "general_log%"; +------------------+-------------------------------------------------------------+ | Variable_name | Value | +------------------+-------------------------------------------------------------+ | general_log | OFF | | general_log_file | /var/lib/mysql/wordpress-ubuntu-s-1vcpu-1gb-amd-ams3-01.log | +------------------+-------------------------------------------------------------+

mysql> SET GLOBAL general_log = 'ON'; mysql> SET GLOBAL general_log = 'OFF';

[gregburg]

But I should apologise, I have 1.5.8.2 version of BuddyBoss Platform

[nickchomey]

Thanks! I'll check that out.

Yeah, you should probably get up to date and test again. But I suspect you'll find the same thing.

[gregburg]

Yep, seems like I'll better wait new version. Already spent so much time looking for reason of slow loading problem(

[nickchomey]

Well, who knows when these issues will be addressed in a new version. But a huge amount of progress has been made between. 1.5.8.2 and 1.7.9. Is there a particular reason you haven't upgraded?

[chetansatasiya]

This PR https://github.com/buddyboss/buddyboss-platform/pull/3055 fix all the remaining BP_Core_Suspend::check_suspended_content( $activity_id, self::$type, true ) caching issues and some of them we have fixed in #3091 PR.

[nickchomey]

@chetansatasiya No, as I commented 11 days ago, you only fixed 4 of 26 instances of bypassing the check_suspended_content cache object. https://github.com/buddyboss/buddyboss-platform/pull/3055#issuecomment-961649693. And there are many other bypasses with check_hidden_content and check_moderation_exists. It is all well documented in this issue and in my comments in that PR. I hope you will be able to rectify it soon!

[chetansatasiya]

@nickchomey As I said here https://github.com/buddyboss/buddyboss-platform/issues/3092#issuecomment-970285238 we will fix all the remaining ones in this https://github.com/buddyboss/buddyboss-platform/pull/3055

[nickchomey]

Sorry, I misunderstood your comment - I thought you were saying that 3055 has already fixed it all, rather than it will fix it all. I'll leave you to your work!

[github-actions[bot]]

We are no longer accepting GitHub issue submissions as our issue tracking and development projects have now moved to JIRA. Click here to read more about it. If you wish to report an issue or bug, please do so from the BuddyBoss Support Portal or Report a Bug directly on our site.

[nickchomey]

@mikejyoung I'm thrilled to see the progress made with the performance updates, but I'm curious as to why this issue (that I cautiously pestered you guys about) was mostly ignored. Most times that check_suspended_content is called, the cache is still bypassed. Likewise for some other functions in this file.

But, even worse than that, the cache is checked and then the wpdb query is run anyway and the cache re-set.

Assuming that there is some legitimate reason to do the bypass (though this isn't at all self-evident), shouldn't the code look something like the following, whereby the cache is only checked if force/bypass = false?

public static function check_suspended_content( $item_id, $item_type, $force = false ) {
        global $wpdb;
        $bp        = buddypress();
        $cache_key = 'bb_check_suspended_content_' . $item_type . '_' . $item_id;       
        if ( false === $force ) {
            $result    = wp_cache_get( $cache_key, 'bp_moderation' );
        }
        if ( false === $result || true === $force ) {
            $result = $wpdb->get_var( $wpdb->prepare( "SELECT DISTINCT id FROM {$bp->moderation->table_name} WHERE item_id = %d AND item_type = %s AND user_suspended = 1", $item_id, $item_type ) ); // phpcs:ignore
            wp_cache_set( $cache_key, $result, 'bp_moderation' );
        }
        return ! empty( $result );
    }

The same would apply for check_hidden_content() and check_blocked_user_content ()

Finally, its not clear why most of the functions in this file use object caching, while others use a static variable

• get_recode_details
• check_blocked_content

and others still have no caching mechanism at all

• get_suspend_detail
• check_suspend_details_exist,

[nickchomey]

@chetansatasiya @mikejyoung

As it turns out, the force/bypass caching described above doesn't actually work.

When the item being checked is not suspended, $wpdb->get_var just returns an empty value (""). Then, when if ( false === $result || true === $force ) { is evaluated, it fails with the following:

Notice: Undefined variable: result in /home/seeingtheforest.net/public_html/wp-content/plugins/buddyboss-platform/bp-moderation/classes/suspend/class-bp-core-suspend.php on line 488

and doesn't even check the true === $force part, so it skips the wpdb->get_var function and wp_cache_set.

I assume there must be some instances in which the bypass is desirable (though surely fewer than what is currently being done), so it would be worth fixing this. I also assume this same problem happens elsewhere throughout the caching mechanism (certainly also with check_hidden_content and check_blocked_user_content)

Also, it appears that the entire cache is flushed with wp_cache_flush() in bb_moderation_clear_suspend_cache(), bb_moderation_clear_status_change_cache() and bb_moderation_clear_delete_cache() (as well as with rather than selectively flushing the relevant cache keys. I just tested by suspending a user and can confirm that the whole cache is flushed when doing that. Doesn't this seem quite extreme?

It is also fully flushed in some other places that don't seem warranted, such as when document folders are saved (bp_nouveau_ajax_document_folder_save):