Sometimes a closed Socket::Shared reads indefinitely and does not close session endpoint (part two)

[KevinEady]

Ref: #101

I'm still having this issue sometimes, at least on Mac OS X (Sonoma 14.0). I'm using commit 82beeea and there doesn't seem to have been any relevant changes since then 82beeea...main

Here's an example server:

hello_debugger.cpp

[I know it's not the best code with the condition variables but it shows the issue I'm facing]

#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <condition_variable>
#include <iostream>
#include <thread>
#include <vector>

#include "dap/io.h"
#include "dap/network.h"
#include "dap/protocol.h"
#include "dap/session.h"

class Event {
 public:
  // wait() blocks until the event is fired.
  void wait();

  // fire() sets signals the event, and unblocks any calls to wait().
  void fire();

 private:
  std::mutex mutex;
  std::condition_variable cv;
  bool fired = false;
};

void Event::wait() {
  std::unique_lock<std::mutex> lock(mutex);
  cv.wait(lock, [&] { return fired; });
}

void Event::fire() {
  std::unique_lock<std::mutex> lock(mutex);
  fired = true;
  cv.notify_all();
}

Event terminate;
std::vector<std::thread> threads;

void my_handler(int s) {
  printf("Caught signal %d\n", s);
  terminate.fire();
}

int main(int argc, char** argv) {
  struct sigaction sigIntHandler;

  sigIntHandler.sa_handler = my_handler;
  sigemptyset(&sigIntHandler.sa_mask);
  sigIntHandler.sa_flags = 0;

  auto _server = dap::net::Server::create();

  auto address = "0.0.0.0";

  auto started = _server->start(
      address, 12345, [&](const std::shared_ptr<dap::ReaderWriter>& rw) {
        threads.emplace_back([=] {
          auto session = dap::Session::create();

          session->onError([&](const char* msg) {
            std::cout << "Session error: " << msg << "\n";
            terminate.fire();
          });

          session->bind(rw,
                        [&]() { std::cout << "Session endpoint closed\n"; });

          terminate.wait();
        });
      });

  if (!started) {
    std::cerr << "Failed to start DAP server.\n";
    _server.reset();
  }

  sigaction(SIGINT, &sigIntHandler, NULL);

  terminate.wait();
  std::cout << "Exiting\n";

  for (auto& thread : threads) {
    thread.join();
  }

  return 0;
}

When I run the server and nc localhost 12345 and then Ctrl+C the nc process, my CPU usage flies through the roof, and it looks like there is still an infinite read:

* thread #6, stop reason = breakpoint 3.1
  * frame #0: 0x000000010008a520 hello_debugger`dap::Socket::Shared::read(this=0x000000013ef044e8, buffer=0x0000000170142c78, bytes=15) at socket.cpp:210:13
    frame #1: 0x000000010000d678 hello_debugger`dap::ContentReader::buffer(this=0x0000000140104390, bytes=15) at content_stream.cpp:151:27
    frame #2: 0x000000010000d8b4 hello_debugger`dap::ContentReader::scan(this=0x0000000140104390, seq="Content-Length:", len=15) at content_stream.cpp:96:10
    frame #3: 0x000000010000d4c0 hello_debugger`dap::ContentReader::scan(this=0x0000000140104390, str="Content-Length:") at content_stream.cpp:107:10
    frame #4: 0x000000010000d1d8 hello_debugger`dap::ContentReader::read(this=0x0000000140104390) at content_stream.cpp:56:10

Again, the recv() call is returning 0 on the closed socket:

cppdap/src/socket.cpp

Lines 208 to 209 in 6464cd7

	auto len =
	recv(s, reinterpret_cast<char*>(buffer), static_cast<int>(bytes), 0);

The loop comes from here, where reader.isOpen() is true, but getPayload() is empty:

cppdap/src/session.cpp

Lines 87 to 91 in 6464cd7

	while (reader.isOpen()) {
	if (auto payload = getPayload()) {
	inbox.put(std::move(payload));
	}
	}

I say sometimes this issue happens because sometimes the session ClosedHandler does execute ... other times, it hangs in this loop forever. Under no circumstance is the session error handler triggered.

[KevinEady]

It look like it stems from errored(SOCKET s) not returning the correct value from getsockopt:

cppdap/src/socket.cpp

Lines 81 to 84 in 6464cd7

	char error = 0;
	socklen_t len = sizeof(error);
	getsockopt(s, SOL_SOCKET, SO_ERROR, &error, &len);
	return error != 0;

-exec bt

* thread #4, stop reason = breakpoint 5.1
  * frame #0: 0x000000010008aaf8 hello_debugger`(anonymous namespace)::errored(s=4) at socket.cpp:84:10
    frame #1: 0x000000010008a2f8 hello_debugger`dap::Socket::Shared::isOpen(this=0x0000000130204348) at socket.cpp:163:36
    frame #2: 0x000000010000d0a4 hello_debugger`dap::ContentReader::isOpen(this=0x00000001302043f0) at content_stream.cpp:40:27
    frame #3: 0x0000000100052738 hello_debugger`(anonymous namespace)::Impl::startProcessingMessages(this=0x00000001302047a8)> const&)::'lambda'()::operator()() const at session.cpp:87:21

error

0 '\0'

---

ETA: I wouldn't say getsockopt() is returning an incorrect value, it just seems that the implementation in cppdap is not robust enough. From my quick researching, a clean closed TCP socket (one that received FIN) will not be in an error state (ie. SO_ERROR sets error to 0). When the connection is closed, the current blocking recv() call returns -1, and any subsequent calls will return 0.

cppdap/src/socket.cpp

Lines 203 to 211 in 6464cd7

	size_t read(void* buffer, size_t bytes) {
	RLock lock(mutex);
	if (s == InvalidSocket) {
	return 0;
	}
	auto len =
	recv(s, reinterpret_cast<char*>(buffer), static_cast<int>(bytes), 0);
	return (len < 0) ? 0 : len;
	}

The implementation of read() completely swallows the error state of recv() returning -1, and just returns to the caller the fact that "no data has been read":

cppdap/src/socket.cpp

Lines 203 to 211 in 6464cd7

	size_t read(void* buffer, size_t bytes) {
	RLock lock(mutex);
	if (s == InvalidSocket) {
	return 0;
	}
	auto len =
	recv(s, reinterpret_cast<char*>(buffer), static_cast<int>(bytes), 0);
	return (len < 0) ? 0 : len;
	}

[KevinEady]

Modifying read():

diff --git a/src/socket.cpp b/src/socket.cpp
index e9402d3..87ce424 100644
--- a/src/socket.cpp
+++ b/src/socket.cpp
@@ -207,7 +207,13 @@ class dap::Socket::Shared : public dap::ReaderWriter {
     }
     auto len =
         recv(s, reinterpret_cast<char*>(buffer), static_cast<int>(bytes), 0);
-    return (len < 0) ? 0 : len;
+
+    if (len < 0) {
+      s = InvalidSocket;
+      return 0;
+    }
+
+    return len;
   }

Seems to solve the problem... is this an acceptable solution? If so, I can create a pull request.