Skip to content
New issue

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

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

Already on GitHub? # to your account

Notices are ignored on session_start() #208

Closed
forrest79 opened this issue Oct 28, 2021 · 4 comments
Closed

Notices are ignored on session_start() #208

forrest79 opened this issue Oct 28, 2021 · 4 comments

Comments

@forrest79
Copy link
Contributor

forrest79 commented Oct 28, 2021

Version: 3.1.3

Bug Description

We're using (phpsession)[https://github.com/phpredis/phpredis/] to handle sessions in Redis with a default locking system. There is a notice raised when a lock can't be acquired (https://github.com/phpredis/phpredis/blob/c588f0308a4d49368a436e660b4a29a0579f802d/redis_session.c#L768). The session_start() returns true even with this notice. Up to version 3.1.2 an exception was thrown, when the lock can't be acquired. Since version 3.1.3 the notice is ignored, session is started and at the end of the request, there is a strange warning logged in Tracy Warning: Unknown: Failed to write session data (redis). Please verify that the current setting of session.save_path is correct (tcp://csfd-service:6380?timeout=3&database=1) with no stack trace.

Steps To Reproduce

Use PHP extension phpredis with similar settings:

php:
  redis.session.locking_enabled: 1
  redis.session.lock_wait_time: 3000 # us
  redis.session.lock_retries: 1

session:
  autoStart: false
  save_handler: redis
  save_path: 'tcp://%caching.redis.host%:%caching.redis.ports.session%?timeout=3&database=%caching.redis.databases.session%'

Start session in PHP:

$container->getByType(Nette\Http\Session::class)->start();

Run two concurrent requests. The second one should receive timeout notice. In 3.1.2 version, we have got an exception, in 3.1.3 is notice ignored.

Expected Behavior

The exception is raised on the session starts if there is a notice raised in session_start().

Possible Solution

Return back Nette\Utils\Callback::invokeSafe for session_start for new minor versions. And possibly add a setting whether a user wants to ignore NOTICES/WARNINGS on session start in a new major version?

@dg
Copy link
Member

dg commented Oct 28, 2021

Now I don't know if it's better to throw an exception and not start the session, or to trigger notices and start the session?

@forrest79
Copy link
Contributor Author

The original solution was best for our scenario - we don't want to start the session when it can't get a lock. So, we're preferring to keep the original solution for 3.1.x branch :-) And in the future branch, put session_start() in some overwritable method?

@dg
Copy link
Member

dg commented Oct 28, 2021

I believed this sentence: „7.1.0: session_start() now returns false and no longer initializes $_SESSION when it failed to start the session“ https://www.php.net/manual/en/function.session-start.php#refsect1-function.session-start-changelog

I'll revert to the original solution and ask you if you can send issue on bugs.php.net, because session_start() should return false in this case.

dg added a commit that referenced this issue Oct 28, 2021
@forrest79
Copy link
Contributor Author

Thanks. I'm not sure, if it's a PHP problem. Maybe I will ask on phpredis first, why notice is raised but session_start returns true.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants