[PATCH] close.2: Suggest shutdown(2) when closing socket from another thread

[PATCH] close.2: Suggest shutdown(2) when closing socket from another thread

[Tomáš Golembiovský]

This is another take on the ancient saga of closing sockets from one
thread while another thread is blocked on recv(2). It all started with
[1,2] and continued by [3]. It was established that this is expected and
long term behavior or Linux and the issue was closed by Michael Kerrisk
by commit c2f15a1349a7271f6c1d81361ec8b256266e1c09.

This is all fine and the patch covered the issue in general terms.
However, it does not mention the specific case of sockets and shutdown,
where the issue can be (at least for the read case) mitigated by proper
shutdown. While one may argue that such information may be implied by
other man pages (perhaps return value of recv(2)) and thus is redundant,
it seems only fair to mention shutdown(2) here as it is only rarely
noted in Linux documentation that properly shutting down both side of
the socket is a good programming practice when dealing with sockets.

As a test program I am attaching the program originally written by Lukas
Czerner. Only with small fixes here and there.

[1] https://lore.kernel.org/linux-man/1314620800-15587-1-git-send-email-lczerner@redhat.com/
[2] https://bugzilla.redhat.com/show_bug.cgi?id=650985
[3] https://bugzilla.kernel.org/show_bug.cgi?id=53781

```c
/**
 * Copyright 2011 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
 * Copyright 2023 (C) Red Hat, Inc., Tomas Golembiovsky <tgolembi@redhat.com>
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License as
 * published by the Free Software Foundation.
 *
 * This program is distributed in the hope that it would be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write the Free Software Foundation,
 * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 */

void *close_socket(void *arg) {
        int sockfd = *(int *)arg;

        sleep(3);
        printf("Thread: closing socket %d\n", sockfd);
//      shutdown(sockfd, SHUT_RDWR);
        close(sockfd);
        return NULL;
}

int client(void) {
        int sockfd;
        int len;
        struct sockaddr_un address;
        int ret;
        char *buffer=malloc(BUFSIZE);
        pthread_t thread;

        sockfd = socket(AF_UNIX, SOCK_STREAM, 0);

        address.sun_family = AF_UNIX;
        strcpy(address.sun_path, "server_socket");
        len = sizeof(address);

        ret = connect(sockfd, (struct sockaddr *)&address, len);
        if (ret == -1) {
                perror("connect");
                return 1;
        }
        printf("client connected\n");

        ret = pthread_create(&thread, NULL, close_socket, (void *)&sockfd);
        if (ret != 0) {
                perror("Creating thread failed");
                return 1;
        }

        while (1) {
                ret = recv(sockfd,buffer,BUFSIZE,0);
                if (ret < 0) {
                        perror("recv");
                        return 1;
                }
                printf("Data received: %s\n", buffer);
                sleep(1);
        }

        close(sockfd);
        return 0;
}

int server(void) {
        char *message="This is the message I am sending to you";
        struct sockaddr_un server_addr, client_addr;
        int server_sockfd, client_sockfd;
        socklen_t server_len, client_len;
        int ret;

        unlink("server_socket");
        server_sockfd = socket(AF_UNIX, SOCK_STREAM, 0);

        server_addr.sun_family = AF_UNIX;
        strcpy(server_addr.sun_path, "server_socket");
        server_len = sizeof(server_addr);
        bind(server_sockfd, (struct sockaddr *)&server_addr, server_len);

        listen(server_sockfd, 5);

        client_len = sizeof(client_addr);
        client_sockfd = accept(server_sockfd,
                (struct sockaddr *)&client_addr, &client_len);

        printf("Server: sending data...\n");
        ret = send(client_sockfd ,message,strlen(message),0);
        if (ret < 0) {
                perror("send");
                return 1;
        }

        /* simulate running server by not closing the client_socket socket */
        return 0;
}

int main() {
        pid_t pid;

        pid = fork();
        if (pid < 0) {
                perror("fork failed");
                exit(1);
        }
        if (pid > 0) {
                printf(" - starting server\n");
                server();
                printf(" - exiting server\n");
                wait(NULL);
        } else {
                sleep(1);
                printf(" - starting client\n");
                client();
                printf(" - exiting client\n");
        }
}
```

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 man2/close.2 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/man2/close.2 b/man2/close.2
index 68211bc58..08c6a0839 100644
--- a/man2/close.2
+++ b/man2/close.2
@@ -145,6 +145,11 @@ Thus, the blocking system call in the first thread may successfully
 complete after the
 .BR close ()
 in the second thread.
+.PP
+When dealing with sockets,
+blocking forever in another thread may be prevented by using
+.BR shutdown (2)
+to shut down both parts of the connection before closing the socket.
 .\"
 .SS Dealing with error returns from close()
 A careful programmer will check the return value of
-- 
2.41.0

Re: [PATCH] close.2: Suggest shutdown(2) when closing socket from another thread

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 6432 bytes --]

Hi Tomáš,

On 2023-08-22 17:19, Tomáš Golembiovský wrote:
> This is another take on the ancient saga of closing sockets from one
> thread while another thread is blocked on recv(2). It all started with
> [1,2] and continued by [3]. It was established that this is expected and
> long term behavior or Linux and the issue was closed by Michael Kerrisk
> by commit c2f15a1349a7271f6c1d81361ec8b256266e1c09.
> 
> This is all fine and the patch covered the issue in general terms.
> However, it does not mention the specific case of sockets and shutdown,
> where the issue can be (at least for the read case) mitigated by proper
> shutdown. While one may argue that such information may be implied by
> other man pages (perhaps return value of recv(2)) and thus is redundant,
> it seems only fair to mention shutdown(2) here as it is only rarely
> noted in Linux documentation that properly shutting down both side of
> the socket is a good programming practice when dealing with sockets.
> 
> As a test program I am attaching the program originally written by Lukas
> Czerner. Only with small fixes here and there.
> 
> [1] https://lore.kernel.org/linux-man/1314620800-15587-1-git-send-email-lczerner@redhat.com/
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=650985
> [3] https://bugzilla.kernel.org/show_bug.cgi?id=53781
> 
> ```c
> /**
>  * Copyright 2011 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
>  * Copyright 2023 (C) Red Hat, Inc., Tomas Golembiovsky <tgolembi@redhat.com>
>  *
>  * This program is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU General Public License as
>  * published by the Free Software Foundation.

No version of the GPL may be understood as GPL-1.0-or-later.  I suggest
specifying.  But rather, we prefer SPDX license tags in this project,
so could you please specify the license as something SPDX?

Thanks,
Alex

>  *
>  * This program is distributed in the hope that it would be useful,
>  * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  * GNU General Public License for more details.
>  *
>  * You should have received a copy of the GNU General Public License
>  * along with this program; if not, write the Free Software Foundation,
>  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>  */
> 
> void *close_socket(void *arg) {
>         int sockfd = *(int *)arg;
> 
>         sleep(3);
>         printf("Thread: closing socket %d\n", sockfd);
> //      shutdown(sockfd, SHUT_RDWR);
>         close(sockfd);
>         return NULL;
> }
> 
> int client(void) {
>         int sockfd;
>         int len;
>         struct sockaddr_un address;
>         int ret;
>         char *buffer=malloc(BUFSIZE);
>         pthread_t thread;
> 
>         sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
> 
>         address.sun_family = AF_UNIX;
>         strcpy(address.sun_path, "server_socket");
>         len = sizeof(address);
> 
>         ret = connect(sockfd, (struct sockaddr *)&address, len);
>         if (ret == -1) {
>                 perror("connect");
>                 return 1;
>         }
>         printf("client connected\n");
> 
>         ret = pthread_create(&thread, NULL, close_socket, (void *)&sockfd);
>         if (ret != 0) {
>                 perror("Creating thread failed");
>                 return 1;
>         }
> 
>         while (1) {
>                 ret = recv(sockfd,buffer,BUFSIZE,0);
>                 if (ret < 0) {
>                         perror("recv");
>                         return 1;
>                 }
>                 printf("Data received: %s\n", buffer);
>                 sleep(1);
>         }
> 
>         close(sockfd);
>         return 0;
> }
> 
> int server(void) {
>         char *message="This is the message I am sending to you";
>         struct sockaddr_un server_addr, client_addr;
>         int server_sockfd, client_sockfd;
>         socklen_t server_len, client_len;
>         int ret;
> 
>         unlink("server_socket");
>         server_sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
> 
>         server_addr.sun_family = AF_UNIX;
>         strcpy(server_addr.sun_path, "server_socket");
>         server_len = sizeof(server_addr);
>         bind(server_sockfd, (struct sockaddr *)&server_addr, server_len);
> 
>         listen(server_sockfd, 5);
> 
>         client_len = sizeof(client_addr);
>         client_sockfd = accept(server_sockfd,
>                 (struct sockaddr *)&client_addr, &client_len);
> 
>         printf("Server: sending data...\n");
>         ret = send(client_sockfd ,message,strlen(message),0);
>         if (ret < 0) {
>                 perror("send");
>                 return 1;
>         }
> 
>         /* simulate running server by not closing the client_socket socket */
>         return 0;
> }
> 
> int main() {
>         pid_t pid;
> 
>         pid = fork();
>         if (pid < 0) {
>                 perror("fork failed");
>                 exit(1);
>         }
>         if (pid > 0) {
>                 printf(" - starting server\n");
>                 server();
>                 printf(" - exiting server\n");
>                 wait(NULL);
>         } else {
>                 sleep(1);
>                 printf(" - starting client\n");
>                 client();
>                 printf(" - exiting client\n");
>         }
> }
> ```
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  man2/close.2 | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/man2/close.2 b/man2/close.2
> index 68211bc58..08c6a0839 100644
> --- a/man2/close.2
> +++ b/man2/close.2
> @@ -145,6 +145,11 @@ Thus, the blocking system call in the first thread may successfully
>  complete after the
>  .BR close ()
>  in the second thread.
> +.PP
> +When dealing with sockets,
> +blocking forever in another thread may be prevented by using
> +.BR shutdown (2)
> +to shut down both parts of the connection before closing the socket.

LGTM.


>  .\"
>  .SS Dealing with error returns from close()
>  A careful programmer will check the return value of

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] close.2: Suggest shutdown(2) when closing socket from another thread

[Tomáš Golembiovský]

On Fri, Aug 25, 2023 at 08:05:01PM +0200, Alejandro Colomar wrote:
> Hi Tomáš,
> 
> On 2023-08-22 17:19, Tomáš Golembiovský wrote:
> > This is another take on the ancient saga of closing sockets from one
> > thread while another thread is blocked on recv(2). It all started with
> > [1,2] and continued by [3]. It was established that this is expected and
> > long term behavior or Linux and the issue was closed by Michael Kerrisk
> > by commit c2f15a1349a7271f6c1d81361ec8b256266e1c09.
> > 
> > This is all fine and the patch covered the issue in general terms.
> > However, it does not mention the specific case of sockets and shutdown,
> > where the issue can be (at least for the read case) mitigated by proper
> > shutdown. While one may argue that such information may be implied by
> > other man pages (perhaps return value of recv(2)) and thus is redundant,
> > it seems only fair to mention shutdown(2) here as it is only rarely
> > noted in Linux documentation that properly shutting down both side of
> > the socket is a good programming practice when dealing with sockets.
> > 
> > As a test program I am attaching the program originally written by Lukas
> > Czerner. Only with small fixes here and there.
> > 
> > [1] https://lore.kernel.org/linux-man/1314620800-15587-1-git-send-email-lczerner@redhat.com/
> > [2] https://bugzilla.redhat.com/show_bug.cgi?id=650985
> > [3] https://bugzilla.kernel.org/show_bug.cgi?id=53781
> > 
> > ```c
> > /**
> >  * Copyright 2011 (C) Red Hat, Inc., Lukas Czerner <lczerner@redhat.com>
> >  * Copyright 2023 (C) Red Hat, Inc., Tomas Golembiovsky <tgolembi@redhat.com>
> >  *
> >  * This program is free software; you can redistribute it and/or
> >  * modify it under the terms of the GNU General Public License as
> >  * published by the Free Software Foundation.
> 
> No version of the GPL may be understood as GPL-1.0-or-later.  I suggest
> specifying.  But rather, we prefer SPDX license tags in this project,
> so could you please specify the license as something SPDX?

I have checked with Lukas and GPL-2.0-or-later will be used. I will post
a new patch with updated file header.

    Tomas