Improving Vanilla: A Cautionary Tale

Unknown

After today's deploy, we started noticing several OOM (out of memory) errors popping up on the staff forum. After some intense troubleshooting, the problem was identified as failures in two separate addons, triggered by a recent community-submitted PR to improve the quality of life for developers. It went something like this:

1. vanilla/vanilla#7705 was merged. Before this, Vanilla was getting responses from the database with resources represented in one of two ways: an array or an object. This caused issues, because they have to be handled two different ways. We wanted to move towards only using arrays and that's where the community PR came into play. It allowed us to treat objects as arrays by creating them as instances of ArrayObject.
2. Vanilla 2.8.dev.1 was deployed.
3. The staff forum began seeing OOM errors on ideation pages. These errors seemed to arise when Vanilla attempted to render the idea's "attachments" area.
4. A long-standing logic error was identified that could lead to infinite recursion when rendering a post's "attachments" on a page. Infinite recursion can very easily exhaust a process's memory. The problem was patched with vanilla/vanilla#7837. It was not immediately apparent why this issue suddenly manifested itself, after having existed that way for years.
5. Additional troubleshooting identified a second issue in the Reactions addon, which was patched with vanilla/internal#1729. It turns out Reactions was looking for a very specific class of object for some of its operations and it wasn't ArrayObject. Because of this, it would mistakenly identify a single row as an array of rows. Instead of looping through several discussions and updating their "UserTags" property, it would loop through the fields of a single discussion and update any values that were an array or object, adding a "UserTags" key to them. The "Attachments" key was one such field, which would go on to trigger the aforementioned infinite recursion issue with attachments. There was an entry in the "Attachments" array that wasn't actually an attachment, due to the errant logic in Reactions.
6. The original PR to improve Vanilla's database results was reverted with vanilla/vanilla#7839. It was apparent there are some addons, and potentially areas of core, that are too sensitive to these changes at the time.

These types of chain-reaction issues aren't very common in Vanilla, but when they do happen, they're generally catastrophic and difficult to troubleshoot. I was genuinely impressed with the speed in which the full scope of this issue was diagnosed and patched.

https://media.giphy.com/media/3rgXBrLlRs4ZlpnVDO/giphy.gif

Unknown

This is the result of our reactions code base being rotten.

$iDs = [];
$userIDs = [];
if (is_object($data) || (is_array($data) && !isset($data[0]))) {
    $data2 = [&$data];
} else {
    $data2 =& $data;
}
foreach ($data2 as $row) {
    if (!$recordType)
        $rT = val('RecordType', $row);
    else
        $rT = $recordType;
    $iD = val($rT.'ID', $row);
    if ($iD)
        $iDs[$rT][$iD] = 1;
}

It took me a lot longer than it should have to understand what this method even expects to take as input. This is ReactionsModal::joinUserTags()

Vanilla Forums

ReactionsModal

🤨

Unknown

https://staff.vanillaforums.com/discussion/comment/1843#Comment_1843

Pretty lame that the indentation is not kept ^

Vanilla Forums

https://staff.vanillaforums.com/discussion/comment/1866#Comment_1866

But wait... did you put it back, or does it work on quotes only?

Unknown

https://staff.vanillaforums.com/discussion/comment/1867#Comment_1867

HAHAHAH! Pretty lame that it doesn't work in the original post!

Unknown

I'd rather people think before they crap over colleagues' code even though I know being critical of others' code is something that is ingrained into the programmer psyche. Just remember that your own code is also never going to be 100% above scrutiny.

Unknown

The intent was not "crap" over anyone but I agree that the way I highlighted the bug can easily be interpreted like so.

I'm sorry.

Unknown

https://staff.vanillaforums.com/discussion/comment/1868#Comment_1868

It's very weird. It shows up in the editor and taking that exact post content and testing it on my localhost shows no issue. Quite curious. It seems to be something happening in post formatting, because updating the comment puts the whitespace back, but it is stripped again on a page reload.

Edit: I've ticked ops for this. My guess is it is a side-effect of cloudflare HTML minification. Ha?

Unknown

https://staff.vanillaforums.com/discussion/comment/1876#Comment_1876

Yeah it's like the formatter is trimming every lines.

Unknown

If anyone was curious about the above code formatting issue: https://github.com/vanilla/vanilla/issues/7852