Detect client disconnection on Server-Sent-Event Server

[gh4ck3r]

I'm making server functionality of Server-Sent-Event on cpp-httplib for my project.
I aware there's example code (ssesvr.cc) in the repository and that's where I began for my own implementation.

In my requirement for the functionality, SSE server should keep client as long as possible even there's nothing to send. For that my implementation dedicate a thread per client and keeps it as long as connected. Problem is server can't aware unexpected disconnection of client without trying to send some data. If that kind of connection is increased, it'll consume entire threadpool after all. I know SSE comment can be used for it. But, I don't want to use it to check connectivity because it makes bunch of useless data to be transferred.

So, I made my own patch to tackle the problem and run existing unittest on Linux. But, it makes failure on ServerTest.ClientStop testcase at following.

cpp-httplib/test/test.cc

Line 1646 in a9cf097

EXPECT_TRUE(sink.is_writable());

I think the EXPECT_TRUE above should be reconsidered. Because, a sink passed into content provider could be unwritable for some reason if it's based on SocketStream.
I modified detail::SocketStream::is_writable() in the patch to check socket via is_socket_alive(). I think, the patch should be applied to SSLSocketStream as well. I want to contribute the patch with relative unittest and patch if you're OK with changing EXPECT_TRUE above and behavior of SocketStream and SSLSocketStream.

[PixlRainbow]

It must be noted that TCP connections have two streams, an upload stream and a download stream, which can be independently disconnected without affecting each other. Clients can disconnect their upload streams and only keep their download streams alive or vice-versa, so it is possible for a socket to be "half-alive".

Note 2: the heartbeat message is not just used to check connectivity. It is actually required to prevent connections from automatically closing due to inactivity. The JavaScript API of many web browsers will close the connection on their own if the connection is idle for too long.

[gh4ck3r]

@PixlRainbow Thanks for the notes. Those are good point to discuss.
I agree with your first note about "half-alive". Originally, I made the patch with send(sock_, nullptr, 0, 0) instead of is_socket_alive(). And it works fine on Linux. What I'm not sure is that whether it works on other OS too. That's why I chose is_socket_alive() which is exists already and make the test pass. Still, I think, send(sock_, nullptr, 0, 0) is better for writable case if it works on other OS too.

For note2, I know heartbeat message is required to prevent inactivity. But, it's not meaning it's suitable for connectivity check. To check the connectivity via heartbeat message it should be sent pretty much frequent. In other words, it makes sense that sending heartbeat message to keep connectivity for every minute but check connectivity same way will be not that useful; IMO, it should be 100 millisecond or less. So, using heartbeat message for checking connection is not tempting for me because i makes event unnecessarily verbose.
To be honest, my very first implementation was using heartbeat message and I don't want to keep the way it is.

[yhirose]

@gh4ck3r, could you send a pull request that includes your suggestion?

[gh4ck3r]

@yhirose, I'm on it.

[yhirose]

Closed by #1371.