silverstripe / silverstripe-versionfeed

Provides an RSS feed for global site changes
BSD 3-Clause "New" or "Revised" License
4 stars 13 forks source link

Symfony error - cannot serialize #59

Closed emteknetnz closed 3 years ago

emteknetnz commented 5 years ago

Navigating to a version feed '/changes' or '/allchanges' on CWP UAT or Prod puts the following error into graylog

error-log.WARNING: Failed to save values {"keys":["changes_13961_26_c12c38119b7c52b015a4d42f87d6fff7"],"exception":"[object] (BadMethodCallException(code: 0): Cannot serialize Symfony\Component\Cache\Simple\FilesystemCache at /sites/mycwpstack/releases/20191007002731/vendor/symfony/cache/Traits/FilesystemCommonTrait.php:121)"} []

In still renders xml to the end user

Similar story with the 'allchanges' endpoint

I cannot replicate on local, even when forcing it to use FilesystemCache

The particular codebase I'm on DOES have custom templates for the changes + allchanges, though I don't think there's anything in there contributing to this error

Seems like the error is starting in from VersionFeedController.php $this->filterContent($key ...

Not sure if this is related, though someone else mentioned to me that In SS4 you basically can’t serialize() any DataObject by default

PR

G-Rath commented 4 years ago

I've got something that looks a lot like this happening on our production stack when we moved to PHP7.3:

error-log.WARNING: Failed to save values {"keys":["allchanges20200106075217public_d01c38119b7c52b015a4d42f87d6fff7"],"exception":"[object] (Exception(code: 0): Serialization of 'Closure' is not allowed at /sites//releases/20191204001228/vendor/symfony/cache/Traits/ApcuTrait.php:101)"} []

It seems to be trying to serialise an SilverStripe\ORM\ArrayList:

{
allchanges20200106075217public_d01c38119b7c52b015a4d42f87d6fff7: Object SilverStripe\ORM\ArrayList
}

It's hammering our error monitoring w/ upwards of 1k+ events a day, but I can't reproduce it locally - I'm happy to work with you folks to try and get this fixed, but I'm out of ideas on my end :/

G-Rath commented 4 years ago

Actually interestingly it seems I can reproduce this, or something very close it locally, but it doesn't appear in the typical logs.

If I enable Sentry on dev, it catches this:

BadMethodCallException: Cannot serialize Symfony\Component\Cache\Simple\FilesystemCache

69 /var/www/vendor/symfony/cache/Traits/FilesystemCommonTrait.php(121): Symfony\Component\Cache\Simple\FilesystemCache::__sleep

68 internal: serialize

67 /var/www/vendor/symfony/cache/Traits/FilesystemTrait.php(98): Symfony\Component\Cache\Simple\FilesystemCache::doSave

66 /var/www/vendor/symfony/cache/Simple/AbstractCache.php(127): Symfony\Component\Cache\Simple\AbstractCache::setMultiple

65 /var/www/vendor/symfony/cache/Simple/AbstractCache.php(77): Symfony\Component\Cache\Simple\AbstractCache::set

64 /var/www/vendor/silverstripe/versioned/src/Caching/ProxyCacheAdapter.php(72): SilverStripe\Versioned\Caching\ProxyCacheAdapter::set

63 /var/www/vendor/silverstripe/versionfeed/src/Filters/CachedContentFilter.php(37): SilverStripe\VersionFeed\Filters\CachedContentFilter::getContent

62 /var/www/vendor/silverstripe/versionfeed/src/VersionFeedController.php(53): SilverStripe\VersionFeed\VersionFeedController::filterContent

61 /var/www/vendor/silverstripe/versionfeed/src/VersionFeedController.php(148): SilverStripe\VersionFeed\VersionFeedController::allchanges

60 internal: call_user_func_array

The above stacktrace looks the same as the one on production, except production doesn't have the first (last?) two frames: #69 & #68.

Mossman1215 commented 4 years ago

Just bumping this from a platform operations perspective. 1,031,203 events of this type in cwp in the last month. Would be nice to get it resolved because it's confusing when other errors are happening to have this one constantly popping up

rafaeldsousa commented 4 years ago

Not a fix, but you can disable it if not needed.

SilverStripe\VersionFeed\VersionFeed:
  changes_enabled: false
  allchanges_enabled: false
maxime-rainville commented 4 years ago

Impact/critical sounds kind of high for something that just pollutes logs.

G-Rath commented 4 years ago

@maxime-rainville

Logs are critical for triaging bugs and responding quickly to events such as site outages, DDoS attacks, security breaches, etc. Log pollution makes it a lot harder to do this job, as it creates noise that teams have to mentally triage & wade through, making it harder to see the forest for the trees so to speak.

Some pollution is ok when it's minor, i.e npm install pollutes your console output with peer dependency warnings, but since these only occur when you run install, their impact is not critical.

However in this case, to quote from my original comment:

It's hammering our error monitoring w/ upwards of 1k+ events a day

We've been lucky in that we actually didn't need this package, so were able to just remove it, but originally we had to disable our error monitoring system for the site in question while we investigated, and if we do require this package in the future (either directly, or it's pulled in by another package) we'll be in trouble.

Mossman1215 commented 4 years ago

yeah it also increases the chances of a disk space starvation outage of logs or the entire system. so having clean logs helps us keep sites up and running.

maxime-rainville commented 4 years ago

Our current definition for impact/critical is:

Broken functionality/experience without any workarounds, or an enhancement that is required to enable a critical task. Typically affecting major usage flows or core interactions. If the issue is type/bug, the fix for it will target all supported minor release lines

https://docs.silverstripe.org/en/4/contributing/code/#github-labels

If someone has time to do a pull request to address this issue, I'd be happy to review it.

emteknetnz commented 4 years ago

Fixed in https://github.com/silverstripe/silverstripe-versioned/pull/305

emteknetnz commented 4 years ago

@maxime-rainville why did you close this?

hadlee-ogilvy commented 3 years ago

@emteknetnz @maxime-rainville did we get anywhere with this issue? I have just found one of our sites having this issue.

Thanks

maxime-rainville commented 3 years ago

I just merged a PR wich should resolve the issue. We can probably do a patch release of the module for people who want a fix right away.

hadlee-ogilvy commented 3 years ago

@maxime-rainville woohoo that sounds great thanks

emteknetnz commented 3 years ago

@hadlee-lineham-wundermanthompson we've just patch releases of the following for silverstripe/versioned that include this fix

Hope this helps Steve.