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 network connect poll interuption handling #16606

Closed
wants to merge 2 commits into from

Conversation

bukka
Copy link
Member

@bukka bukka commented Oct 25, 2024

When connecting to socket, it is possible to get EINTR. In such case, there should be an another attempt to connect if we are not over the timeout. The timeout should be adjusted accordingly in that case.

This should fix phpredis/phpredis#1881 . It is inspired by redis/hiredis#1213 that had a similar issue.

@bukka bukka changed the base branch from master to PHP-8.2 November 24, 2024 16:45
bukka added a commit to bukka/php-src that referenced this pull request Nov 24, 2024
When connecting to socket, it is possible to get EINTR. In such case,
there should be an another attempt to connect if we are not over the
timeout. The timeout should be adjusted accordingly in that case.

Closes phpGH-16606
@bukka bukka force-pushed the streams_net_connect_eintr branch from 104773e to f0c72c4 Compare November 24, 2024 16:45
@bukka bukka marked this pull request as ready for review November 24, 2024 16:45
@bukka
Copy link
Member Author

bukka commented Nov 24, 2024

This should be now ready for review. I will see if there is anyway how I could create an EINTR at least manually and test it a bit but code wise, it should be ready.

@nielsdos
Copy link
Member

Note that gettimeofday() may not be the appropriate method here because that is not a monotonic clock. I.e. your time difference may become negative
In the patch I was developing I was using php_hrtime_current to compute the time difference.

@bukka
Copy link
Member Author

bukka commented Nov 24, 2024

Isn't that covered by timercmp check? I used what was already there actually and this case is pretty unlikely because EINTR happens quite rarely so even not changing timeout is probably not the end of the world. I wouldn't worry about some small inaccuracy though. Unless it can somehow overflow ofc?

@nielsdos
Copy link
Member

It's indeed covered by timercmp. In the case of a clock time update you can get a shorter timeout than expected, which is not 100% correct but I see your point that this is rare.

Unless it can somehow overflow ofc?

I don't think so.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Looks fine, just one silly nit

main/network.c Outdated Show resolved Hide resolved
When connecting to socket, it is possible to get EINTR. In such case,
there should be an another attempt to connect if we are not over the
timeout. The timeout should be adjusted accordingly in that case.

Closes phpGH-16606
@bukka bukka force-pushed the streams_net_connect_eintr branch 2 times, most recently from f65bff1 to 319fc93 Compare November 28, 2024 16:23
@bukka bukka force-pushed the streams_net_connect_eintr branch from 319fc93 to 687b335 Compare November 28, 2024 16:24
@bukka
Copy link
Member Author

bukka commented Nov 28, 2024

@nielsdos Just a note that I struggled a bit to find some good way how to test those paths reliably. So I decided to introduce unit testing setup using cmocka to mock the libc functions and be able to cover those paths. I will ping other devs for feedback as well.

@bukka
Copy link
Member Author

bukka commented Nov 29, 2024

I merged just the code change without the tests - the tests are now in #16987

@nielsdos
Copy link
Member

@nielsdos Just a note that I struggled a bit to find some good way how to test those paths reliably. So I decided to introduce unit testing setup using cmocka to mock the libc functions and be able to cover those paths. I will ping other devs for feedback as well.

Thanks, I'll have a look tonight.
I previously used an interposer in zend_test with an INI toggle to achieve something similar:

php-src/ext/zend_test/test.c

Lines 1499 to 1514 in 284c4e3

#ifdef HAVE_COPY_FILE_RANGE
/**
* This function allows us to simulate early return of copy_file_range by setting the limit_copy_file_range ini setting.
*/
#ifdef __MUSL__
typedef off_t off64_t;
#endif
PHP_ZEND_TEST_API ssize_t copy_file_range(int fd_in, off64_t *off_in, int fd_out, off64_t *off_out, size_t len, unsigned int flags)
{
ssize_t (*original_copy_file_range)(int, off64_t *, int, off64_t *, size_t, unsigned int) = dlsym(RTLD_NEXT, "copy_file_range");
if (ZT_G(limit_copy_file_range) >= Z_L(0)) {
len = ZT_G(limit_copy_file_range);
}
return original_copy_file_range(fd_in, off_in, fd_out, off_out, len, flags);
}
#endif

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.

pconnect: Operation now in progress
2 participants