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

[BUG]: Random exceptions when retrieving data using Stream + Json #16339

Closed
krazzer opened this issue May 17, 2023 · 1 comment · Fixed by #16345 or #16347
Closed

[BUG]: Random exceptions when retrieving data using Stream + Json #16339

krazzer opened this issue May 17, 2023 · 1 comment · Fixed by #16345 or #16347
Assignees
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium

Comments

@krazzer
Copy link

krazzer commented May 17, 2023

Describe the bug
The Json serialiser uses if unlikely when determining if json_decode gives an error or not. This is faster, but can give unexpected results. https://igoro.com/archive/fast-and-slow-if-statements-branch-prediction-in-modern-processors/.

This is no problem for caching scenario's, but the Stream storage adapter may as well be used for permanent storage.

To Reproduce
This is not easy to reproduce with just a simple script, you need to run a lot of writing / reading of Streams over some time. Then exceptions will appear.

Expected behavior
Not have random errors when reading stream data with the Json adapter.
Of course this is easily fixed by simply using if instead of if unlikely, but this probably more a design philosophy question.

Details

  • Phalcon version: 4.2 (But still present in 5.x)
  • PHP Version: 7.4
  • Operating System: Linux
  • Installation type: Cloudlinux
  • Server: Apache
@krazzer krazzer added bug A bug report status: unverified Unverified labels May 17, 2023
@niden niden self-assigned this May 30, 2023
@niden niden mentioned this issue May 30, 2023
5 tasks
@niden niden linked a pull request May 30, 2023 that will close this issue
5 tasks
@niden niden mentioned this issue May 30, 2023
5 tasks
@niden niden linked a pull request May 30, 2023 that will close this issue
5 tasks
@niden
Copy link
Member

niden commented May 30, 2023

This has been addressed in #16347

Thank you @krazzer

@niden niden added status: medium Medium 5.0 The issues we want to solve in the 5.0 release and removed status: unverified Unverified labels May 30, 2023
@niden niden added this to Phalcon v5 May 30, 2023
@niden niden moved this to Implemented in Phalcon v5 May 30, 2023
@niden niden moved this from Implemented to Released in Phalcon v5 Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium
Projects
Status: Released
Development

Successfully merging a pull request may close this issue.

2 participants