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

Avoid unneeded syscall on socket close #292

Merged
merged 1 commit into from
May 20, 2022

Conversation

clue
Copy link
Member

@clue clue commented May 20, 2022

This simple PR removes an unneeded syscall on socket close. In this state, the socket is already in non-blocking mode anyway and about to be closed anyway, so setting non-blocking mode again provides very little value whatsoever. The comment above this command suggests this might have been needed for some (unknown) legacy version, but careful testing and our test suite confirms this behavior can not be exhibited on any known versions. Given the socket is closed immediately after this call, there's little chance this could cause unwanted side effects in any real world applications in either case.

This change does not affect performance in any noticeable way, but helps avoid unneeded syscalls in an attempt to ease debugging and improve overall performance (see reactphp/http#455). For a single HTTP request, this means the following syscalls can be eliminated:

pselect6(5, [4], [], [], NULL, NULL)    = 1 (in [4])
poll([{fd=4, events=POLLIN|POLLERR|POLLHUP}], 1, 0) = 1 ([{fd=4, revents=POLLIN}])
accept(4, {sa_family=AF_INET6, sin6_port=htons(54462), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "::ffff:127.0.0.1", &sin6_addr), sin6_scope_id=0}, [128 => 28]) = 3
fcntl(3, F_GETFL)                       = 0x2 (flags O_RDWR)
fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK)    = 0
fcntl(3, F_GETFL)                       = 0x802 (flags O_RDWR|O_NONBLOCK)
fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK)    = 0
pselect6(5, [3 4], [], [], NULL, NULL)  = 1 (in [3])
poll([{fd=3, events=POLLIN|POLLPRI|POLLERR|POLLHUP}], 1, 0) = 1 ([{fd=3, revents=POLLIN}])
recvfrom(3, "G", 1, MSG_PEEK, NULL, NULL) = 1
recvfrom(3, "GET / HTTP/1.1\r\nHost: localhost:"..., 65536, 0, NULL, NULL) = 78
poll([{fd=3, events=POLLIN|POLLPRI|POLLERR|POLLHUP}], 1, 0) = 0 (Timeout)
recvfrom(3, 0x7fceeb63d066, 65458, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
getpeername(3, {sa_family=AF_INET6, sin6_port=htons(54462), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "::ffff:127.0.0.1", &sin6_addr), sin6_scope_id=0}, [128 => 28]) = 0
getsockname(3, {sa_family=AF_INET6, sin6_port=htons(8080), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "::ffff:127.0.0.1", &sin6_addr), sin6_scope_id=0}, [128 => 28]) = 0
gettimeofday({tv_sec=1652528350, tv_usec=915571}, NULL) = 0
gettimeofday({tv_sec=1652528350, tv_usec=915621}, NULL) = 0
gettimeofday({tv_sec=1652528350, tv_usec=915765}, NULL) = 0
pselect6(5, [3 4], [3], [], NULL, NULL) = 1 (out [3])
sendto(3, "HTTP/1.1 200 OK\r\nContent-Type: t"..., 150, 0, NULL, 0) = 150
pselect6(5, [3 4], [], [], NULL, NULL)  = 1 (in [3])
poll([{fd=3, events=POLLIN|POLLPRI|POLLERR|POLLHUP}], 1, 0) = 1 ([{fd=3, revents=POLLIN}])
recvfrom(3, "", 1, MSG_PEEK, NULL, NULL) = 0
recvfrom(3, "", 65536, 0, NULL, NULL)   = 0
shutdown(3, SHUT_RDWR)                  = 0
- fcntl(3, F_GETFL)                       = 0x802 (flags O_RDWR|O_NONBLOCK)
- fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK)    = 0
close(3)                                = 0

pselect6(5, [4], [], [], NULL, NULL)    = …

This changeset only removes some internal logic and does not affect the public API, so it should be safe to apply. The test suite confirms this has 100% code coverage.

Refs reactphp/http#455

@WyriHaximus WyriHaximus merged commit dd3c325 into reactphp:1.x May 20, 2022
@clue clue deleted the unneeded-nonblock branch May 21, 2022 07:51
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.

3 participants