Add platform agnostic socket error macros and error details helper by sunjayBhatia · Pull Request #11565 · envoyproxy/envoy

sunjayBhatia · 2020-06-11T22:16:06Z

Commit Message:
Add platform agnostic socket error macros and error details helper

WSAE* error codes are a disjoint set from POSIX error codes (which are
still defined on Windows for non winsock2 errors) so we define macros
that map to these error codes on Windows and the usual POSIX error
codes on other platforms
do not use strerror to get error details on Windows, instead use
FormatMessage and add utility function to replace strerror usage
replace usual error code macros with new SOCKET_ERROR_ macros in
applicable locations where they come from socket operations
any differences in error code returned by socket operations continue
to be handled with an #ifdef to make the differences between platforms
clear (e.g. when SOCKET_ERROR_AGAIN is returned by Windows but
SOCKET_ERROR_IN_PROGRESS is returned by other platforms)
Fix small UBSAN failure in integration test library

Additional Description:
Risk Level: Low (should be a 1-1 replacement for Linux etc.)
Testing: Adds a unit test for Windows and modifies some to use the new macros etc.
Docs Changes: N/A
Release Notes: N/A
Fixes #11298

- WSAE* error codes are a disjoint set from POSIX error codes (which are still defined on Windows for non winsock2 errors) so we define macros that map to these error codes on Windows and the usual POSIX error codes on other platforms - do not use strerror to get error details on Windows, instead use FormatMessage and add utility function to replace sterror usage - replace usual error code macros with new SOCKET_ERROR_ macros in applicable locations where they come from socket operations - any differences in error code returned by socket operations continue to be handled with an #ifdef to make the differences between platforms clear (e.g. when SOCKET_ERROR_AGAIN is returned by Windows but SOCKET_ERROR_IN_PROGRESS is returned by other platforms) Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

repokitteh-read-only · 2020-06-11T22:16:14Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11565 was opened by sunjayBhatia.

see: more, trace.

sunjayBhatia · 2020-06-11T22:24:02Z

We experimented with and considered other options to address the general issue described below:

Problems:

there are specific winsock2 error codes, that do not correspond to the regular POSIX errno values
- common case: EAGAIN would typically signal a program to repeat an operation, etc. but with winsock2 it's equivalent to WSAEWOULDBLOCK
EAGAIN/WSAEWOULDBLOCK happen very frequently so frequent allocations of error objects is not ideal
we need to be able to report the exact error code in logs (be sure not to replace a specific winsock2 error code with a translation of any kind)
we need to be able to report the exact error description string in logs (strerror may not cut it on Windows)

Options:

just add #ifdef case statements for WSA error codes when we translate to Envoy IoErrorCode
- can use a macro in the error class to correctly generate the correct case statement
- getIoSocketEagainInstance should also create an error with WSAEWOULDBLOCK instead of EAGAIN
- easy, but messier? mixes up Windows and other platform specifics together in the IoSocketError class
- ignores tests directly accessing OsSysCallsImpl, those tests and others who generate error objects in mocks etc. would still need to be updated with ifdefs
- in places where we look at the errno coming from a sys call, use new macros for comparing constants (a la SOCKET_VALID() macro or SOCKET_ERROR macro)
#define new macros for the errors we care about, use those exclusively (the option we chose)
- POSIX: #define ERROR_AGAIN EAGAIN
- WINDOWS: #define ERROR_AGAIN WSAEWOULDBLOCK
- should get rid of many #ifdefs required but could be brittle if developers are frequently changing code and forget to use the defined error codes
- still may be some platform specifics where we need to check additional error codes on a platform (e.g. the error from a syscall is slightly different and doesn't map to the same ERROR_* for all platforms), however calling these differences out may be good as it ensures developers can directly see it when making changes
translate WSA errors to POSIX errno equivalents in OsSysCallsImpl results
- translation table from WSA to posix errors
- always pass around the POSIX error code
- This will likely become confusing for users who see a POSIX error code or error string in logs and don't understand the mapping
- Optimizations:
  - co-mingle required NT and WSA errors by expected frequency
  - static map of WSA error to POSIX error for constant time access in os sys calls
change IoError interface completely to no longer use getErrorCode() or use IoErrorCode enum to interrogate error specifics
- instead interrogate error object with 'isError()' methods, e.g. isErrorAgain() for EAGAIN/WSAEWOULDBLOCK
- can have a different implementation per platform as needed
- can cleanly have multiple errors that resolve to the same "is error this or that" logic, such as EAGAIN/EWOULDBLOCK/EINPROGRESS (from the man 7 socket page: "-1 is returned with errno set to EAGAIN or EWOULDBLOCK, or EINPROGRESS")
- requires a bigger refactor of the IoError interface
- does not cover cases where we are calling syscalls directly and looking at errnos, this only helps the code that uses IoErrors

cc @wrowe @nigriMSFT @davinci26

source/common/common/utility.h

sunjayBhatia · 2020-06-11T22:27:33Z

source/common/network/connection_impl.cc

  } else {
    ASSERT(result.rc_ == -1);
-    if (result.errno_ == EINPROGRESS) {
+#ifdef WIN32


This is an example of an error code that is different on Windows vs POSIX, personally I think it may be valuable to keep the #ifdef to ensure developers understand the differences between connect() on different platforms, but we could potentially abstract this error check in a different way, open to feedback about this

I like the approach as long as we don't have too many instances of the same syscall

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

source/common/filesystem/file_shared_impl.cc

davinci26

LGTM

source/common/common/utility.cc

davinci26 · 2020-06-12T21:33:41Z

Should we also start removing the fail on windows tag with this change to have a sense of where we are at with getting all the tests running?

wrowe · 2020-06-13T01:08:36Z

When you resolve a test failure, remove the corresponding fails_on_windows tag.

You can simply change your bazel test command to ignore that flag to review newly passing tests, or only build tests with that flag present to discover newly passing tests.

In any case removing that flag without fixing the bug will break CI, so please don't do that.

ggreenway · 2020-06-16T15:57:50Z

@junr03 Can you take a pass on this?

- pre-allocating error message buffer we may fail to fetch error details for errors with long descriptions - revert IoFileError.getErrorDetails() to use strerror, we will take another PR to refactor Filesystem Impl on Windows to use Win32 APIs not POSIX Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

junr03

a few comments

junr03 · 2020-06-17T22:54:13Z

source/common/common/utility.cc


+const std::string errorDetails(int error_code) {
+#ifndef WIN32
+  return strerror(error_code);


Can we document why strerror cannot be used?

added a comment describing below

source/common/common/utility.h

junr03 · 2020-06-17T22:57:19Z

source/common/common/utility.h

+ * @param int error code
+ * @return const std::string error detail description
+ */
+const std::string errorDetails(int error_code);


can we somehow enforce that new code does not use strerror? clang-tidy rule?

I'm not super experienced with clang-tidy and couldn't find a check that could do this, do you know of one? We could add to the format checks potentially instead?

we're thinking of doing a format check instead, since we also have a couple places where we intentionally left strerror b/c there is another refactor required before we get rid of it (the filesystem impl on Windows uses the POSIX subsystem which is problematic for error handling purely Windows errors), we can use clang-format off/on to guard around those blocks that need it still

#11655 for the filesystem impl issue

/wait

adding checks for strerror in clang format

added a new format check, please take a look!

junr03 · 2020-06-17T23:01:16Z

source/extensions/filters/listener/http_inspector/http_inspector.cc

      os_syscalls.recv(socket.ioHandle().fd(), buf_, Config::MAX_INSPECT_SIZE, MSG_PEEK);
  ENVOY_LOG(trace, "http inspector: recv: {}", result.rc_);
-  if (result.rc_ == -1 && result.errno_ == EAGAIN) {
+  if (SOCKET_FAILURE(result.rc_) && result.errno_ == SOCKET_ERROR_AGAIN) {


out of curiosity, do we have any more places with the old-style comparison that should be converted to SOCKET_FAILURE?

I found a few more and converted them, it was tough to think of a format or other automation to detect it as it is dependent on if the return code comes from a socket function, lmk if you think there is a good way to auto detect this

junr03 · 2020-06-17T23:03:36Z

include/envoy/common/platform.h

 #define ENVOY_SHUT_RDWR SD_BOTH

+// Mapping winsock2 errors to common error names
+#define SOCKET_ERROR_AGAIN WSAEWOULDBLOCK


I am wondering if we should document this decision a bit more inline, i.e., expand on the thinking about defining envoy-neutral socket errors and maping them to platform specific definitions?

Added some more content here, lmk if this is sufficient

awesome, this is great!

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

wrowe · 2020-06-22T19:41:09Z

/retest envoy-presubmit

for DNS failure

repokitteh-read-only · 2020-06-22T19:41:12Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #11565 (comment) was created by @wrowe.

see: more, trace.

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

sunjayBhatia · 2020-06-23T22:06:40Z

@junr03 should we similarly ignore the kqueue code in clang-tidy (like is done for Windows)? That seems to be the last error remaining in the existing checks

cc @junr03 @ggreenway if you have thoughts on this

No longer the last clang-tidy error (we will fix those others in a commit soon), but still relevant

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com> Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

sunjayBhatia · 2020-06-25T15:18:49Z

~~not quite sure yet how to fix this ubsan error, though it appears to be unrelated to the changes in this PR~~

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior test/integration/integration.cc:470:45 in

~~It appears ubsan is still evaluating this and checking the result of test_server_->counter(success) for nullptr even though the first part of the or already does so~~:

envoy/test/integration/integration.cc

Line 470 in d0b4b4f

test_server_->counter(success)->value() < concurrency_) &&

The ubsan error in this failure could be fixed by grabbing the counter once instead of each time it is needed, ~~would appreciate your thoughts @ggreenway @junr03 if we should fix this here or open another issue~~, should be fixed in the most recent commit

see https://dev.azure.com/cncf/4684fb3d-0389-4e0b-8251-221942316e06/_apis/build/builds/43031/logs/104 for a raw log that hopefully does not go away with future pushes (AZP build: https://dev.azure.com/cncf/envoy/_build/results?buildId=43031&view=logs&jobId=930b2ee4-0ea7-5b74-a645-6bc26962bc06)

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

davinci26 · 2020-06-26T20:49:16Z

@junr03 @ggreenway can we get this PR merged with master 🚀

This PR unblock various tests that are failing on windows and I have been working on top of it for some time now and it would be awesome if we have it in master.

junr03

lgtm. @ggreenway do you mind giving a final look?

junr03 · 2020-06-26T21:16:19Z

include/envoy/common/platform.h

  __pragma(pack(pop))

-using ssize_t = ptrdiff_t;
+typedef ptrdiff_t ssize_t;


nit: I believe we bias to using using.

We are aware of this and wrote the original solve. We've run across an edge case where using was insufficient, and these are global C declarations of posix to be emulated correctly on Windows (We did joke about making it a #define, obviously not seriously:)

I believe we changed those to typedef instead of using to appease clang-tidy in this commit: 6ab3de8, if there is a clang-tidy trick we don't know of (besides just turning tidy off for that section) we could, change to, that would be great to know

junr03 · 2020-06-26T21:16:27Z

include/envoy/common/platform.h

 #define ENVOY_SHUT_RDWR SD_BOTH

+// Mapping winsock2 errors to common error names
+#define SOCKET_ERROR_AGAIN WSAEWOULDBLOCK


awesome, this is great!

ggreenway · 2020-06-26T22:36:30Z

lgtm. @ggreenway do you mind giving a final look?

I'm a bit backed up with some end-of-quarter stuff. I'm going to unassign myself. @envoyproxy/senior-maintainers PTAL.

mattklein123

Nice!

sunjayBhatia requested review from PiotrSikora, alyssawilk, htuch, lizan and mattklein123 as code owners June 11, 2020 22:16

repokitteh-read-only bot added the api label Jun 11, 2020

sunjayBhatia commented Jun 11, 2020

View reviewed changes

source/common/common/utility.h Show resolved Hide resolved

sunjayBhatia commented Jun 11, 2020

View reviewed changes

sunjayBhatia mentioned this pull request Jun 11, 2020

WIP: Resolve WSAE* errors on Windows into expected Posix error states #11298

Closed

Fix compilation of filesystem test

e8ef294

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia commented Jun 12, 2020

View reviewed changes

source/common/filesystem/file_shared_impl.cc Outdated Show resolved Hide resolved

davinci26 reviewed Jun 12, 2020

View reviewed changes

source/common/common/utility.cc Outdated Show resolved Hide resolved

htuch removed the api label Jun 12, 2020

mattklein123 assigned ggreenway Jun 14, 2020

ggreenway assigned junr03 Jun 16, 2020

repokitteh-read-only bot added waiting api labels Jun 16, 2020

wrowe and others added 2 commits June 16, 2020 14:15

Merge remote-tracking branch 'origin/master' into io-error-macros

15a3b17

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

repokitteh-read-only bot removed the waiting label Jun 16, 2020

sunjayBhatia and others added 2 commits June 16, 2020 17:12

Fix some clang tidy errors

7d5c44d

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

Revert file_shared_impl.cc fully back to master for less of a diff

52d8fed

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

junr03 reviewed Jun 17, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into io-error-macros

dd3353e

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

repokitteh-read-only bot removed the waiting label Jun 19, 2020

sunjayBhatia added 3 commits June 22, 2020 10:02

Fix python format

b31bb8d

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Merge remote-tracking branch 'origin/master' into io-error-macros

81dba22

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Add missing include

6749cae

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Merge remote-tracking branch 'origin/master' into io-error-macros

57342bb

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

wrowe and others added 4 commits June 23, 2020 18:16

clang-tidy fixes

d5a0445

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

Exclude kqueue filesystem impl from clang-tidy

a3b6665

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Merge remote-tracking branch 'origin/master' into io-error-macros

7cf5c23

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia mentioned this pull request Jun 24, 2020

Windows: untag passing tests #11741

Merged

Merge remote-tracking branch 'origin/master' into io-error-macros

bfc5cd7

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

wrowe and others added 5 commits June 25, 2020 12:33

Attemp to fix UBSAN error

c700536

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

Start with null and reset on each loop iteration

67b6092

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Merge remote-tracking branch 'origin/master' into io-error-macros

55435c6

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Actually, dont waste 10ms on each test

81cccb8

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Merge remote-tracking branch 'origin/master' into io-error-macros

6b51299

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

junr03 reviewed Jun 26, 2020

View reviewed changes

ggreenway removed their assignment Jun 26, 2020

mattklein123 self-assigned this Jun 27, 2020

mattklein123 approved these changes Jun 28, 2020

View reviewed changes

mattklein123 merged commit 9e3dbff into envoyproxy:master Jun 28, 2020

wrowe deleted the io-error-macros branch June 29, 2020 18:19

Conversation

sunjayBhatia commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jun 11, 2020

Uh oh!

sunjayBhatia commented Jun 11, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davinci26 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davinci26 commented Jun 12, 2020

Uh oh!

wrowe commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggreenway commented Jun 16, 2020

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wrowe commented Jun 22, 2020

Uh oh!

repokitteh-read-only bot commented Jun 22, 2020

Uh oh!

sunjayBhatia commented Jun 23, 2020

Uh oh!

sunjayBhatia commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davinci26 commented Jun 26, 2020

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Jun 26, 2020

Uh oh!

mattklein123 left a comment

sunjayBhatia commented Jun 11, 2020 •

edited

Loading

wrowe commented Jun 13, 2020 •

edited

Loading

sunjayBhatia commented Jun 25, 2020 •

edited

Loading