Alternative check for sockets in win32/select.c, fixes #19722 #19725
Conversation
|
Can you please resolve the merge conflict? Thanks! |
|
Sorry, it's because the branch is based on PHP-8.4, I don't know why it conflicts with master. I'll resolve it later :) |
I have verified this fixes the assert issue locally, but I'm not sure if it may have side effects. Hopefully the build will tell.
dktapps
force-pushed
the
stream-select-assert
branch
from
0641281 to
fabe490
Compare
| sock_max_fd = i; | ||
| } | ||
| } else { | ||
| handles[n_handles] = (HANDLE)(uintptr_t)_get_osfhandle(i); |
There was a problem hiding this comment.
I wonder wether an INVALID_HANDLE_VALUE can be returned here, and what to do in this case. Previously, that would have been treated as socket. Maybe we should stick with this; otherwise, it might be prudent to ignore such invalid handles.
There was a problem hiding this comment.
Good catch, I hadn't considered that. I'm doubtful assuming it's a socket is a good idea since that might cause issues with socket-specific parts of the code. Probably best to just skip them.
WaitForMultipleObjects is told how many elements to read, and it'll never read the element pointed to by n_handles, so we don't need to initialize this.
There was a problem hiding this comment.
We can merge into master, since the assertion only affects debug builds. The fix for handling INVALID_HANDLE_VALUE may be valuable on lower branches too though. Perhaps we can merge first into master and if no problems arise merge it later into lower branches?
|
I've merged this now and will keep an eye on this. Thanks! |
3 participants
I have verified this fixes the assert issue locally, but I'm not sure if it may have side effects. Hopefully the build will tell.