Task #87959

Bring back igbinary support

Added by Michael Stucki over 1 year ago. Updated 9 months ago.

Status:
Rejected
Priority:
Should have
Assignee:
-
Category:
-
Target version:
-
Start date:
2019-03-20
Due date:
% Done:

0%

Estimated time:
TYPO3 Version:
10
PHP Version:
Tags:
Complexity:
Sprint Focus:

Description

I just realized that php-redis in Ubuntu 18.04 has a dependency on php-igbinary.
This made me wonder because I remember it was used in the past in the caching framework but got removed again later:

- https://forge.typo3.org/issues/74501
- https://review.typo3.org/c/Packages/TYPO3.CMS/+/47157

The changelog for the removal commit says:

[TASK] Remove igbinary from cache framework

The patch removes igbinary serializer support from the cache framework.
When igbinary support was added back then in PHP 5.3 times, it had the
potential to serialize data quicker than the default serializer.

Unfortunately, other promises aren't held:
* The module found no general acceptance within the PHP community and
  isn't supported very well. PHP 7 is still not officially supported.
* Last release at the time of this writing was in 2014-08
* The module must still be compiled from source and no recent
  distribution ever packaged it by default
* The maintenance load on devOps side is high: The module must be
  recompiled with each minor php release
* In case the module is not updated, it throws errors at a central place
  of the system and can easily brick a whole instance
* The module found no huge acceptance by hosters and is only very rarely
  used in real life instances
* A performance impact is only measurable in very small and highly
  specialized use cases, it typically plays no role in casual frontend
  or backend requests.

We've seen several life systems in the wild lately with sloppy hosters
not maintaining the igbinary module within their infrastructure properly.
The current implementation with VariableFrontend dynamically detecting
and then force using the module leads to hard crashes in those situations.
The main issue is that serializing is done via PHPs serializer and
unserializing using igbinary then fails, effectively rendering the
entire installation bricked.
To come by those situations, it is considered more important to deliver
a stable product than a product that is quicker in more theoretical use
cases.

Thus, the support for this module is kicked from the standard cache
frontend. In case igbinary still gives significant boost for specialized
specific instances, an admin can still configure a VariableFrontend that
uses this serializer.

As it looks to me, this is no longer valid, and the module is actively improved:

- https://pecl.php.net/package/redis
- https://pecl.php.net/package-changelog.php?package=redis

Also, it is now required by php-redis:

- https://github.com/phpredis/phpredis/commit/3baccddd11e4c8526f46884d73c44a63b02ecf6f
- see changelog above at version 3.1.1
- Ubuntu 18.04 package: https://launchpad.net/ubuntu/bionic/+source/php-redis

Therefore I suggest to check again if the situation has changed and if php-igbinary support should be added (with a configuration flag?) again.


Related issues

Related to TYPO3 Core - Task #74501: Remove igbinary from cache frameworkClosed2016-03-08

Actions
#1

Updated by Michael Stucki over 1 year ago

  • Related to Task #74501: Remove igbinary from cache framework added
#2

Updated by Michael Stucki over 1 year ago

  • Description updated (diff)
#3

Updated by Michael Stucki over 1 year ago

  • Description updated (diff)
#4

Updated by Michael Stucki over 1 year ago

Christian, Mathias, Morton: You were involved in removing this feature. Are you aware of any issues with this?

#5

Updated by Michael Stucki over 1 year ago

Can someone take a look at this please?

#6

Updated by Mathias Schreiber over 1 year ago

I'm not a pro in PHP internals but to me it looks like it is an option in the php redis module.
It reads to me (and I could be wrong) that if the igbinary module is there, then it's a dependency.

So it's about balancing risk and reward.

The risks have been outlined in the original commit - I consider the risk "high" because the error is very hard to track down and renders an entire install bricked.
I did several tests back then testing out the reward (aka the increase in speed) and it used to be a single digit improvement - and even that was when I created unrealistic scenarios with thousands of records being serialized and unserialized.
The team collectively decided that these usecases are very edgy and might (positively) affect a very low number of installations.

That's the current state, so far, so good.

Neither Christian or me can make this a priority at the moment, so if someone wants to step in and run tests to determine the reward of bringing back igbinary as well as taking over the time-critical fixing in case issues or regressions show up, I don't object to bringing igbinary back.

#7

Updated by Michael Stucki 9 months ago

  • Status changed from New to Rejected

As discussed with Benni, this will be closed.

Also available in: Atom PDF