Is Sink.is_writable() thread-safe

[ochafik]

Hi, I see a fair bit of work has been done to make httplib threadsafe, wondering if that extends to Sink.is_writable()?

Context: trying to detect closed connections (in a set_content_provider setup) from another thread in llama.cpp. Our writes are still happening only on the http thread, but knowing when to cancel the processing would be amazing.

[yhirose]

@ochafik thanks for the interesting question.

According to the following article, POSIX socket operations like send/recv are atomic.
https://stackoverflow.com/questions/1981372/are-parallel-calls-to-send-recv-on-the-same-socket-valid

is_writable method consists of the subsequent select_write and is_socket_alive calls.

cpp-httplib/httplib.h

Lines 5819 to 5822 in 10d68cf

	inline bool SocketStream::is_writable() const {
	return select_write(sock_, write_timeout_sec_, write_timeout_usec_) > 0 &&
	is_socket_alive(sock_);
	}

select_write doesn't write, but checks the write status of the socket with poll or select. I don't know they are atomic, but I think they are because of consistency with other socket system calls.

cpp-httplib/httplib.h

Lines 3104 to 3130 in 10d68cf

	inline ssize_t select_write(socket_t sock, time_t sec, time_t usec) {
	#ifdef CPPHTTPLIB_USE_POLL
	struct pollfd pfd_read;
	pfd_read.fd = sock;
	pfd_read.events = POLLOUT;
	auto timeout = static_cast<int>(sec * 1000 + usec / 1000);
	return handle_EINTR([&]() { return poll(&pfd_read, 1, timeout); });
	#else
	#ifndef _WIN32
	if (sock >= FD_SETSIZE) { return -1; }
	#endif
	fd_set fds;
	FD_ZERO(&fds);
	FD_SET(sock, &fds);
	timeval tv;
	tv.tv_sec = static_cast<long>(sec);
	tv.tv_usec = static_cast<decltype(tv.tv_usec)>(usec);
	return handle_EINTR([&]() {
	return select(static_cast<int>(sock + 1), nullptr, &fds, nullptr, &tv);
	});
	#endif
	}

is_socket_alive ends up calling recv eventually, but it's an atomic call as mentioned in the above stackoverflow post.

cpp-httplib/httplib.h

Lines 3189 to 3198 in 10d68cf

	inline bool is_socket_alive(socket_t sock) {
	const auto val = detail::select_read(sock, 0, 0);
	if (val == 0) {
	return true;
	} else if (val < 0 && errno == EBADF) {
	return false;
	}
	char buf[1];
	return detail::read_socket(sock, &buf[0], sizeof(buf), MSG_PEEK) > 0;
	}

So it seems like it's ok to call is_writable in different threads, but I am not 100% sure according to my limited knowledge.
Also WinSock2 on Windows may behave differently than Linux or macOS.

Did you find any actual problem in your project? If so I may be able to try to reproduce it on my machine.
Anyway, sorry that I am not able to give you a concrete answer, but hope my answer give you something helpful.

[ochafik]

@yhirose thank you so much for the detailed research and prompt reply!

No actual problem but the question was raised by @ngxson in a code review, hoping to avoid running into problems if that PR gets merged :-D

[ngxson]

Hmm in this case, if only taking into account POSIX (linux, bsd), then is_writable() is an atomic operation and should block until the last operation is finished.

For llama.cpp, this can have negative impact on inference if there're select_write being called multiple times from other threads (so one call has to wait for the other to finish). What I'm wondering is: can we only use select_read to determine if the connection is still alive. It seems to me that select_read will not compromise on performance, since it's never be used after HTTP request being processed.

[ochafik]

What I'm wondering is: can we only use select_read to determine if the connection is still alive.

select_read alone didn't seem to cut it but is_socket_alive (= select_read + read_socket) seems to work -> #1956

Also WinSock2 on Windows may behave differently than Linux or macOS.

It looks like winsock should be reasonably threadsafe, with just undefined ordering behaviour for concurrent writes (not our case). MSDN doesn't spell much out though.