Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Nov 18, 2018

This PR changes the file descriptor from the array size to random bytes. Compatibility is guaranteed through random_compat, although PHP5 support is running out in one month.

Using random bytes guarantees no clashing file descriptors, that means re-using and overwriting a currently active file descriptor.

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@CharlotteDunois Thanks for working on this and filing this PR, but the motivation is currently unclear to me, so perhaps you can help clarify.

What is this ID used for? It's my understanding that this is just an arbitrary identifier that needs to be unique at a given time so that two calls to open() do not assign the same ID? If I'm not mistaken, it seems to be save to reuse previous IDs once they are no longer in use.

It's my understanding that using random numbers as IDs here only reduces the probability of a clash and doesn't really address the underlying issue. As an alternative, how about simply appending to this list and just let PHP add incrementing IDs (for example https://github.com/clue/reactphp-mq/blob/master/src/Queue.php#L248,L250)? What do you think about this?

@ghost
Copy link
Author

ghost commented Dec 12, 2018

PHP does not re-use numeric integers. If we wanted to re-use IDs we'd have to make our own management for this. The only numeric key PHP re-uses is the PHP_INT_MAX, but only if the key is not already present, otherwise an error will be thrown.

@ghost ghost closed this Jan 14, 2019
@ghost ghost deleted the child-process-random branch January 14, 2019 04:52
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants