Skip to content

Use zend_long for resource ID #7436

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

Closed
wants to merge 1 commit into from
Closed

Conversation

nikic
Copy link
Member

@nikic nikic commented Aug 31, 2021

Currently, resource IDs are limited to 32-bits. As resource IDs
are not reused, this means that resource ID overflow for
long-running processes is very possible.

This patch switched resource IDs to use zend_long instead, which
means that on 64-bit systems, 64-bit resource IDs will be used.
This makes resource ID overflow practically impossible.

The tradeoff is an 8 byte increase in zend_resource size.

@nikic
Copy link
Member Author

nikic commented Aug 31, 2021

@dstogov I'd like to consider making this ABI breaking change for PHP-8.1, while we still have the opportunity.

Currently, resource IDs are limited to 32-bits. As resource IDs
are not reused, this means that resource ID overflow for
long-running processes is very possible.

This patch switched resource IDs to use zend_long instead, which
means that on 64-bit systems, 64-bit resource IDs will be used.
This makes resource ID overflow practically impossible.

The tradeoff is an 8 byte increase in zend_resource size.
@nikic nikic force-pushed the resource-id-size branch from cb0b037 to edd56c0 Compare August 31, 2021 09:25
@dstogov
Copy link
Member

dstogov commented Aug 31, 2021

I think, a better solution would be reusing IDs in the same way as as we do with objects.

@nikic
Copy link
Member Author

nikic commented Aug 31, 2021

@dstogov I'm concerned that reusing IDs will cause backwards compatibility issues. Current code can assume that resource IDs are unique for the whole request lifetime.

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.

2 participants