Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wrowe opened this issue Apr 21, 2020 · 9 comments
Open
Assignees
Labels
area/windows no stalebot Disables stalebot from closing an issue tech debt

Comments

@wrowe
Copy link
Contributor

wrowe commented Apr 21, 2020

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 #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
Copy link
Contributor Author

wrowe commented Apr 21, 2020

cc @sunjayBhatia

@wrowe
Copy link
Contributor Author

wrowe commented Apr 21, 2020

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

microsoft/WSL#4240

@nigriMSFT
Copy link
Contributor

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
Copy link
Member

davinci26 commented Nov 19, 2020

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

  1. watcher_impl
  2. signal_impl (see [Win32 Signals] Add term and ctrl-c signal handlers #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
Copy link
Member

/assign @sunjayBhatia

@sunjayBhatia
Copy link
Member

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
Copy link
Member

davinci26 commented Jan 7, 2021

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
Copy link
Contributor Author

wrowe commented Jan 7, 2021

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?

@wrowe wrowe modified the milestone: windows-beta Jan 7, 2021
@sunjayBhatia
Copy link
Member

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

@sunjayBhatia sunjayBhatia removed their assignment Feb 22, 2021
@wrowe wrowe removed this from the windows-ga milestone Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/windows no stalebot Disables stalebot from closing an issue tech debt
Projects
None yet
Development

No branches or pull requests

5 participants