Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Make registering serializers easier for shared session module #7715

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TysonAndre
Copy link
Contributor

@TysonAndre TysonAndre commented Dec 4, 2021

Why: PHP's serializer is a text format, and for many use case it (1) uses more storage space for serialized data (increasing data storage needs for sessions), and (2) is slower than PECLs such as https://github.com/igbinary/igbinary#igbinary or https://github.com/msgpack/msgpack-php/ at serializing/unserializing data for most use cases. Making it easier for end users to install modules and use them as session serializers would help save space and improve performance for users.

Additionally, this PR passes the address of the PHP reference value (IS_REFERENCE) PS(http_session_vars)
as an extra argument to the serializer and unserializer,
so that extensions don't need to directly depend on symbols
found in the session shared module (ps_globals.http_session_vars).

(Grepping the tarballs for the top hundreds of PECL extensions by downloads,
PECLs such as igbinary, msgpack, and hprose provide custom session serializers
that are more space efficient than PHP's built-in serializers for most use cases.
I'm a maintainer of igbinary.)

Previously, when --enable-session=shared is used instead of
--enable-session=static, external session serializers such as igbinary and
msgpack would either

  1. load session serializers because it would result in php failing to load
    all of that serializer's functionality if session wasn't loaded, or if session
    was going to be loaded after the serializer.

    This was based on the behavior of the now-removed wddx extension for php

  2. Crash/fail to load if the shared library is loaded if the session module
    hadn't been loaded first.

  • php_session_register_serializer and PS(http_session_vars) would be undefined
    C symbols and cause the serializer shared library to fail to load.

Examples of issues caused:

Because many users of PHP use a package manager instead of building php
themselves, and package managers often publish php-session as a shared library that
must be installed separately, this means that many users are unable to use
custom serialization methods with php's native session handlers if session isn't
statically compiled into php.
(https://www.php.net/manual/en/session.configuration.php#ini.session.serialize-handler)

This PR exposes a new function pointer in the module globals that can be
accessed through the session module in HashTable *module_registry if
session_module && session_module->module_started.
Existing PECL releases will be unaffected and can continue to use
php_session_register_serializer without changes.

ZTS: ((php_ps_globals*) (*((void ***) tsrm_get_ls_cache()))[TSRM_UNSHUFFLE_RSRC_ID(*session_module->globals_id_ptr)]); NTS: session_module->globals_ptr`

Why not user-defined serializers/unserializers

This PR is deliberately a change with no/minimal impact on existing PECL releases or userland code.

This does not propose adding a user-defined serializer. That was already proposed in https://wiki.php.net/rfc/user_defined_session_serializer and fell short of the 2/3 yes vote requirement.

https://externals.io/message/97279#97304

The other RFC had the following possible objections:

  • Lack of details in the RFC document itself on error handling design
  • Arguments over the proper way to signal errors when a user-defined session serializer/unserializer failed (throwing, emitting a notice, etc.)
  • Easier for buggy session serializers/unserializers to be written in userland
  • Users preferring alternative API designs, e.g. adding a new interface option with save(string $uuid, array $data) and read(string $uuid): array $data as an alternative class to SessionHandlerInterface

TysonAndre added a commit to TysonAndre/igbinary that referenced this pull request Dec 4, 2021
…dule

**This depends on the proposal for PHP 8.2 (php/php-src#7715)
to allow safely registering native session serializers even if the session
module isn't statically compiled into php
(without crashing if the session module isn't loaded or is loaded in the
wrong order, after the extension providing the serializer)
@TysonAndre TysonAndre force-pushed the session-support branch 7 times, most recently from 0204f87 to a6ca581 Compare December 4, 2021 16:18
TysonAndre added a commit to TysonAndre/igbinary that referenced this pull request Dec 4, 2021
…dule

**This depends on the proposal for PHP 8.2 (php/php-src#7715)
to allow safely registering native session serializers even if the session
module isn't statically compiled into php
(without crashing if the session module isn't loaded or is loaded in the
wrong order, after the extension providing the serializer)
Additionally, this PR passes the PHP reference value `PS(http_session_vars)`
as an extra argument to the serializer and unserializer,
so that extensions don't need to directly depend on symbols
found in the session shared module (`ps_globals.http_session_vars`).

(Grepping the tarballs for the top hundreds of PECL extensions by downloads,
PECLs such as igbinary, msgpack, and hprose provide custom session serializers
that are more space efficient than PHP's built-in serializers for most use cases.
I'm a maintainer of igbinary.)

Previously, when `--enable-session=shared` is used instead of
`--enable-session=static`, external session serializers such as igbinary and
msgpack would either

1. load session serializers because it would result in php failing to load
   all of that serializer's functionality if session wasn't loaded, or if session
   was going to be loaded after the serializer.

   This was based on the behavior of the now-removed `wddx` extension for php
2. Crash/fail to load if the shared library is loaded if the session module
   hadn't been loaded first.

- php_session_register_serializer and PS(http_session_vars) would be undefined
  C symbols and cause the serializer shared library to fail to load.

Examples of issues caused:
- https://github.com/msgpack/msgpack-php/issues?q=is%3Aissue+session+is%3Aclosed
  for `undefined symbol` issues (ps_globals, etc)
- igbinary/igbinary#116

Because many users of PHP use a package manager instead of building php
themselves, and package managers often publish php-session as a shared library that
must be installed separately, this means that many users are unable to use
custom serialization methods with php's native session handlers if session isn't
statically compiled into php.
(https://www.php.net/manual/en/session.configuration.php#ini.session.serialize-handler)

- e.g. https://pkgs.alpinelinux.org/package/edge/community/x86/php8-session
  is required by php8-pecl-msgpack because of this.
- Some other OSes will statically compile session into php to avoid
  this (e.g. Windows, seemingly homebrew for Mac, Ubuntu).

This PR exposes a new function pointer in the module globals that can be
accessed through the session module in `HashTable *module_registry` if
`session_module && session_module->module_started`.
Existing PECL releases will be unaffected and can continue to use
`php_session_register_serializer` without changes.

ZTS: `((php_ps_globals*) (*((void ***) tsrm_get_ls_cache()))[TSRM_UNSHUFFLE_RSRC_ID(*session_module->globals_id_ptr)]);
NTS: `session_module->globals_ptr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants