Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/424#discussion_r28205714
--- Diff: lib/cpp/src/thrift/transport/TSocket.cpp ---
@@ -493,29 +585,9 @@ uint32_t TSocket::read(uint8_t* buf, uint32_t len) {
goto try_again;
}
-#if defined __FreeBSD__ || defined __MACH__
if (errno_copy == THRIFT_ECONNRESET) {
- /* shigin: freebsd doesn't follow POSIX semantic of recv and fails
with
- * THRIFT_ECONNRESET if peer performed shutdown
- * edhall: eliminated close() since we do that in the destructor.
- */
return 0;
}
-#endif
-
-#ifdef _WIN32
--- End diff --
Hi Roger, thanks for inspecting this closely. I determined that if one
called read() on a closed socket, it was not returning 0 like it should. It
turns out that the block seen above which was limiting the logic for handling
ECONNRESET to __FreeBSD__ and __MACH__ should be ubiquitous. If a read()
errors with ECONNRESET the proper response is to return 0 indicating EOF.
The WIN32 block is the same, given THRIFT_ECONNRESET == WSAECONNRESET when
_WIN32 is enabled. As such, now all platforms respond to connection reset
during read with 0, meaning EOF.
Does the CI build test framework test Windows and run the CMake tests? If
so, then the TServerIntegrationTest will test your concern. I did not run this
code on Windows to make sure it builds or passes tests, but I can do that on
Monday if needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
