Replace pipe() with os_sys_calls_impl::socketpair(AF_UNIX,...) in buffer tests

[wrowe]

I think as all of these tests are buffer tests and not network tests, having them all use socket pair seems fine to me so let's just do that?

Originally posted by @mattklein123 in https://github.com/envoyproxy/envoy/pull/10822

The underlying issue is that Unix can only use an AF_UNIX socketpair, while the current os_sys_calls_impl socketpair on Windows implements only AF_INET or AF_INET6.

In order to drop the pipe() in favor of a common use of socketpair() on both windows and linux in the test/common/buffer/watermark_buffer_test.cc and test/common/buffer/owned_impl_test.cc sources, we will need to implement AF_UNIX socketpair in source/common/api/win32/os_sys_calls_impl.cc - and must do so without using anonymous AF_UNIX sockets, which are unsupported on Windows (as on MacOS.)

[wrowe]

cc @sunjayBhatia

[wrowe]

Note: Microsoft is aware of the issue and the issues it raises for implementing socketpair()

https://github.com/microsoft/WSL/issues/4240

[nigriMSFT]

For reference, libevent has a Windows AF_UNIX socketpair implementation: evutil_win_socketpair_afunix, https://github.com/libevent/libevent/blob/master/evutil.c

Not saying its great, but its an example.

[davinci26]

Think more about this and I realized that we are using socketpair in production in:

1. watcher_impl
2. signal_impl (see #13954)

It would be a perf win not only for tests but also for production code to go through UDS instead of loopback. Just as a note that we should also change this these places when we get this

[sunjayBhatia]

/assign @sunjayBhatia

[sunjayBhatia]

So a few points to clarify before doing this (which is also why I would say to remove this from the beta milestone as well)

Windows doesn't have support for unnamed UDS, so we would need to have a filepath they live at which is especially interesting if we are going to use UDS in the watcher and signals prod code. It seems Envoy has stayed out of the business of saving state etc. to disk outside of what users configure Envoy to output to (logs etc.)

Where would this be?

• Some temp dir? Which one? What happens if some default we chose does not exist?
• Make it configurable? Where would that configuration go?

[davinci26]

Some temp dir? Which one? What happens if some default we chose does not exist?

I think that this could go to %APPDATA% on Windows. It's an internal cache that the application is using. Edit: We can use the same primitive for this as dotnet core Path.GetTempPath

Make it configurable? Where would that configuration go?

Do you think this needs to be configurable in the first version? For this type of configuration even an environment variable could do it. I might be wrong but I don't think the user would want to change this from the data plane.

[wrowe]

So a few points to clarify before doing this (which is also why I would say to remove this from the beta milestone as well)

I agree with removing the beta milestone, I'll do so now.

During beta...

It would be a perf win not only for tests but also for production code to go through UDS instead of loopback

That's an untested assertion. I think we can begin at 1.17.0 as a baseline using loopback, introduce this feature after resolving the path question and potential socketpair conflicts and prove up what the performance change is.

We also are left with a great number of lingering unix sockets running the envoy test framework. It seems we will want some reliable mechanism to purge the unused unix socketpairs after closure, before testing this in production?

[sunjayBhatia]

un-assigning myself as its unlikely I will get to this soon

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.