-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpFoundation] use atomic writes in MockFileSessionStorage #39816
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also handle concurency in read method: is_file($filePath) ? file_get_contents($filePath)
given the file could be deleted in the meantime (see #39167 (comment))
Unless we have reports that this happens IRL, I think we should fix as little as possible. The linked report is only about non-atomic writes. I'd wait before doing more on this topic. |
I felt like the linked comment reported by @mpdude were a real case. |
It is, and the message there is about "Notice: unserialize()". That's unrelated to the |
Over here, I also mentioned that the |
OK, thanks for the link. So you have processes that concurrently destroy the session? |
0d7d19c
to
7331030
Compare
Now updated to guard against concurrent |
Regarding @jderusse's comment above, the set_error_handler(static function () {});
try {
$this->data = [];
$this->data = unserialize(file_get_contents($filePath));
} finally {
restore_error_handler();
} But regardless of that, 👍 and thanks for the fix! |
7331030
to
5290e97
Compare
code updated |
@nicolas-grekas i just merged your PR into my largest test-set, and it's still passing! Thank you! |
Instead of #39808