Git fork

Merge branch 'jk/fsmonitor-event-listener-race-fix'

On macOS, fsmonitor can fall into a race condition that results in
a client waiting forever to be notified for an event that have
already happened. This problem has been corrected.

* jk/fsmonitor-event-listener-race-fix:
fsmonitor: initialize fs event listener before accepting clients
simple-ipc: split async server initialization and running

+98 -18
+3 -3
builtin/fsmonitor--daemon.c
··· 1208 1208 * system event listener thread so that we have the IPC handle 1209 1209 * before we need it. 1210 1210 */ 1211 - if (ipc_server_run_async(&state->ipc_server_data, 1212 - state->path_ipc.buf, &ipc_opts, 1213 - handle_client, state)) 1211 + if (ipc_server_init_async(&state->ipc_server_data, 1212 + state->path_ipc.buf, &ipc_opts, 1213 + handle_client, state)) 1214 1214 return error_errno( 1215 1215 _("could not start IPC thread pool on '%s'"), 1216 1216 state->path_ipc.buf);
+6
compat/fsmonitor/fsm-listen-darwin.c
··· 516 516 } 517 517 data->stream_started = 1; 518 518 519 + /* 520 + * Our fs event listener is now running, so it's safe to start 521 + * serving client requests. 522 + */ 523 + ipc_server_start_async(state->ipc_server_data); 524 + 519 525 pthread_mutex_lock(&data->dq_lock); 520 526 pthread_cond_wait(&data->dq_finished, &data->dq_lock); 521 527 pthread_mutex_unlock(&data->dq_lock);
+6
compat/fsmonitor/fsm-listen-win32.c
··· 741 741 start_rdcw_watch(data->watch_gitdir) == -1) 742 742 goto force_error_stop; 743 743 744 + /* 745 + * Now that we've established the rdcw watches, we can start 746 + * serving clients. 747 + */ 748 + ipc_server_start_async(state->ipc_server_data); 749 + 744 750 for (;;) { 745 751 dwWait = WaitForMultipleObjects(data->nr_listener_handles, 746 752 data->hListener,
+3 -2
compat/simple-ipc/ipc-shared.c
··· 16 16 struct ipc_server_data *server_data = NULL; 17 17 int ret; 18 18 19 - ret = ipc_server_run_async(&server_data, path, opts, 20 - application_cb, application_data); 19 + ret = ipc_server_init_async(&server_data, path, opts, 20 + application_cb, application_data); 21 21 if (ret) 22 22 return ret; 23 23 24 + ipc_server_start_async(server_data); 24 25 ret = ipc_server_await(server_data); 25 26 26 27 ipc_server_free(server_data);
+23 -5
compat/simple-ipc/ipc-unix-socket.c
··· 328 328 int back_pos; 329 329 int front_pos; 330 330 331 + int started; 331 332 int shutdown_requested; 332 333 int is_stopped; 333 334 }; ··· 824 825 /* 825 826 * Start IPC server in a pool of background threads. 826 827 */ 827 - int ipc_server_run_async(struct ipc_server_data **returned_server_data, 828 - const char *path, const struct ipc_server_opts *opts, 829 - ipc_server_application_cb *application_cb, 830 - void *application_data) 828 + int ipc_server_init_async(struct ipc_server_data **returned_server_data, 829 + const char *path, const struct ipc_server_opts *opts, 830 + ipc_server_application_cb *application_cb, 831 + void *application_data) 831 832 { 832 833 struct unix_ss_socket *server_socket = NULL; 833 834 struct ipc_server_data *server_data; ··· 887 888 server_data->accept_thread->server_socket = server_socket; 888 889 server_data->accept_thread->fd_send_shutdown = sv[0]; 889 890 server_data->accept_thread->fd_wait_shutdown = sv[1]; 891 + 892 + /* 893 + * Hold work-available mutex so that no work can start until 894 + * we unlock it. 895 + */ 896 + pthread_mutex_lock(&server_data->work_available_mutex); 890 897 891 898 if (pthread_create(&server_data->accept_thread->pthread_id, NULL, 892 899 accept_thread_proc, server_data->accept_thread)) ··· 918 925 return 0; 919 926 } 920 927 928 + void ipc_server_start_async(struct ipc_server_data *server_data) 929 + { 930 + if (!server_data || server_data->started) 931 + return; 932 + 933 + server_data->started = 1; 934 + pthread_mutex_unlock(&server_data->work_available_mutex); 935 + } 936 + 921 937 /* 922 938 * Gently tell the IPC server treads to shutdown. 923 939 * Can be run on any thread. ··· 933 949 934 950 trace2_region_enter("ipc-server", "server-stop-async", NULL); 935 951 936 - pthread_mutex_lock(&server_data->work_available_mutex); 952 + /* If we haven't started yet, we are already holding lock. */ 953 + if (server_data->started) 954 + pthread_mutex_lock(&server_data->work_available_mutex); 937 955 938 956 server_data->shutdown_requested = 1; 939 957
+44 -4
compat/simple-ipc/ipc-win32.c
··· 371 371 HANDLE hEventStopRequested; 372 372 struct ipc_server_thread_data *thread_list; 373 373 int is_stopped; 374 + 375 + pthread_mutex_t startup_barrier; 376 + int started; 374 377 }; 375 378 376 379 enum connect_result { ··· 526 529 return ret; 527 530 } 528 531 532 + static void wait_for_startup_barrier(struct ipc_server_data *server_data) 533 + { 534 + /* 535 + * Temporarily hold the startup_barrier mutex before starting, 536 + * which lets us know that it's OK to start serving requests. 537 + */ 538 + pthread_mutex_lock(&server_data->startup_barrier); 539 + pthread_mutex_unlock(&server_data->startup_barrier); 540 + } 541 + 529 542 /* 530 543 * Thread proc for an IPC server worker thread. It handles a series of 531 544 * connections from clients. It cleans and reuses the hPipe between each ··· 549 562 550 563 memset(&oConnect, 0, sizeof(oConnect)); 551 564 oConnect.hEvent = hEventConnected; 565 + 566 + wait_for_startup_barrier(server_thread_data->server_data); 552 567 553 568 for (;;) { 554 569 cr = wait_for_connection(server_thread_data, &oConnect); ··· 752 767 return hPipe; 753 768 } 754 769 755 - int ipc_server_run_async(struct ipc_server_data **returned_server_data, 756 - const char *path, const struct ipc_server_opts *opts, 757 - ipc_server_application_cb *application_cb, 758 - void *application_data) 770 + int ipc_server_init_async(struct ipc_server_data **returned_server_data, 771 + const char *path, const struct ipc_server_opts *opts, 772 + ipc_server_application_cb *application_cb, 773 + void *application_data) 759 774 { 760 775 struct ipc_server_data *server_data; 761 776 wchar_t wpath[MAX_PATH]; ··· 787 802 strbuf_addstr(&server_data->buf_path, path); 788 803 wcscpy(server_data->wpath, wpath); 789 804 805 + /* 806 + * Hold the startup_barrier lock so that no threads will progress 807 + * until ipc_server_start_async() is called. 808 + */ 809 + pthread_mutex_init(&server_data->startup_barrier, NULL); 810 + pthread_mutex_lock(&server_data->startup_barrier); 811 + 790 812 if (nr_threads < 1) 791 813 nr_threads = 1; 792 814 ··· 837 859 return 0; 838 860 } 839 861 862 + void ipc_server_start_async(struct ipc_server_data *server_data) 863 + { 864 + if (!server_data || server_data->started) 865 + return; 866 + 867 + server_data->started = 1; 868 + pthread_mutex_unlock(&server_data->startup_barrier); 869 + } 870 + 840 871 int ipc_server_stop_async(struct ipc_server_data *server_data) 841 872 { 842 873 if (!server_data) ··· 850 881 * We DO NOT attempt to force them to drop an active connection. 851 882 */ 852 883 SetEvent(server_data->hEventStopRequested); 884 + 885 + /* 886 + * If we haven't yet told the threads they are allowed to run, 887 + * do so now, so they can receive the shutdown event. 888 + */ 889 + ipc_server_start_async(server_data); 890 + 853 891 return 0; 854 892 } 855 893 ··· 899 937 server_data->thread_list = std->next_thread; 900 938 free(std); 901 939 } 940 + 941 + pthread_mutex_destroy(&server_data->startup_barrier); 902 942 903 943 free(server_data); 904 944 }
+13 -4
simple-ipc.h
··· 179 179 * When a client IPC message is received, the `application_cb` will be 180 180 * called (possibly on a random thread) to handle the message and 181 181 * optionally compose a reply message. 182 + * 183 + * This initializes all threads but no actual work will be done until 184 + * ipc_server_start_async() is called. 182 185 */ 183 - int ipc_server_run_async(struct ipc_server_data **returned_server_data, 184 - const char *path, const struct ipc_server_opts *opts, 185 - ipc_server_application_cb *application_cb, 186 - void *application_data); 186 + int ipc_server_init_async(struct ipc_server_data **returned_server_data, 187 + const char *path, const struct ipc_server_opts *opts, 188 + ipc_server_application_cb *application_cb, 189 + void *application_data); 190 + 191 + /* 192 + * Let an async server start running. This needs to be called only once 193 + * after initialization. 194 + */ 195 + void ipc_server_start_async(struct ipc_server_data *server_data); 187 196 188 197 /* 189 198 * Gently signal the IPC server pool to shutdown. No new client