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

Fix: only flush temporary unreferenced streams #1351

Merged
merged 4 commits into from
Jan 26, 2025

Conversation

AlliBalliBaba
Copy link
Collaborator

This PR refines the fix in #1350.

I'll do some more testing, but explicitly checking if the stream is referenced as a zval will prevent issues with curl/openssl on a Laravel application I've been testing with.

Copy link
Collaborator

@withinboredom withinboredom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes a lot of sense, I think. If I understand correctly, we basically only delete temp streams? Do we need to consider pure memory streams as well, or does this cover it?

@AlliBalliBaba
Copy link
Collaborator Author

This should cover all temporary streams created via php_stream_temp_create_ex. The important part is checking for stream->__exposed.

Tests actually also pass if all we do is check for stream->__exposed == 0. So any stream that is unreachable by PHP:

if (val->type == stream_type) {
  php_stream *stream = (php_stream *)val->ptr;
  if (stream->__exposed == 0) {
    zend_list_close(val);
    zend_list_delete(val);
  }
}

The other checks are only there for safety and might not be necessary.

@AlliBalliBaba
Copy link
Collaborator Author

AlliBalliBaba commented Jan 26, 2025

In other words, any stream reachable by PHP is also reachable by GC. We want to clean up the streams not reachable by the GC.

@AlliBalliBaba AlliBalliBaba merged commit 7e39e0a into main Jan 26, 2025
55 checks passed
@AlliBalliBaba AlliBalliBaba deleted the fix/only-flush-temporary-streams branch January 26, 2025 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants