@tsteur opened this Issue on December 4th 2019 Member

image

Noticed this in the plugin settings table. Basically there is an array stored but we expect a string or integer. Not sure how this happens as I think it does set only a string in the application. Maybe some race condition?

This breaks the archiving for us.

Looking only quickly at the code I don't really understand how this could happen: https://github.com/matomo-org/matomo/blob/3.13.0/plugins/Tour/Engagement/Challenge.php#L142-L148

@tsteur commented on December 4th 2019 Member

I have an idea how this happens...

basically, at the same time two processes are inserting the same data:
https://github.com/matomo-org/matomo/blob/3.13.0/core/Settings/Storage/Backend/PluginSettingsTable.php#L96-L98

Then because the same key exists twice, it turns it into an array here: https://github.com/matomo-org/matomo/blob/3.13.0/core/Settings/Storage/Backend/PluginSettingsTable.php#L135-L139

And next time the value gets saved it will be stored as the array again...

@tsteur commented on December 4th 2019 Member

Wonder if we want to use locks to avoid this issue in general around this code: https://github.com/matomo-org/matomo/blob/3.13.0/core/Settings/Storage/Backend/PluginSettingsTable.php#L78-L101

Otherwise could do transactions I suppose but maybe not every DB supports it.

We could also execute all inserts at once (bulk insert) but this wouldn't fully avoid the issue as basically there is still a gap between delete an insert.

Another possibility would be here to iterate over all settings, and converted any array to the related value if the key has the skipped or completed appendix: https://github.com/matomo-org/matomo/blob/3.13.0/plugins/Tour/Engagement/Challenge.php#L86

Most of the plugin settings usually would not have concurrency issues but this tour plugin settings is a bit different. So we could potentially avoid most of the issues by making sure we convert any array to a string for these keys.

There might be also more options using other locks that we have etc. Not sure which one is best. Fixing the plugin settings class be certainly best.

@tsteur commented on December 5th 2019 Member

image

just noticed the same issue with bingsiteurl... not sure this is also some race condition or maybe a different issue @sgiehl ?

googlevideokeywords was defined twice as well
image

This Issue was closed on December 22nd 2019
Powered by GitHub Issue Mirror