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.

Sign up for GitHub

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

Jump to bottom

[EventEngine] Support AF_UNIX for windows #34801

Closed
wants to merge 2 commits into from
Closed

[EventEngine] Support AF_UNIX for windows #34801

wants to merge 2 commits into from

Conversation

youyuanwu
Copy link
Contributor

#22285
Unix domain socket has been added to windows: AF_UNIX comes to Windows

Golang net pkg has adopted this long ago:
https://go-review.googlesource.com/c/go/+/125456
https://go-review.googlesource.com/c/sys/+/132555
grpc-go already support this.

AF_UNIX on windows is seamlessly integrated with winsock API.
The modification needed are:

  • Set the right address family AF_UNIX depending on the address config, instead of using AF_INET6 all the time.
  • Ignore socket options for tcp.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 26, 2023 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

@youyuanwu youyuanwu marked this pull request as ready for review October 28, 2023 22:17
@markdroth markdroth assigned drfloob and unassigned markdroth Oct 30, 2023
@youyuanwu
Copy link
Contributor Author

@drfloob Can you review?

@drfloob
Copy link
Member

drfloob commented Nov 7, 2023

Hi @youyuanwu. This feature has been in discussion for a while. In short, we need to consider whether unix sockets can be treated identically within gRPC across platforms, or if there are subtle semantic differences that require a bit more scrutiny. There are a few folks reviewing the concept now, and we'll get back with you soon.

Thank you for the contribution regardless!

@youyuanwu
Copy link
Contributor Author

The PR has been tested manually with grpc-go, which already supports AF_UNIX on windows.
run_tests.py with c++ tests passes locally.

@lostobject
Copy link

I have also done some testing of this with my own C++ server and C# client (grpc-dotnet which already supports AF_UNIX). No issues found.

@drfloob
Copy link
Member

drfloob commented Dec 19, 2023

Sorry for the delay, @youyuanwu. We've gotten consensus that this seems pretty safe to implement, so we can continue with the code review. Unfortunately, I'm going to be out for the remainder of the year, so we'll have to pick it up in January, but I'll do a quick first pass today.

drfloob
drfloob reviewed Dec 19, 2023
View reviewed changes
src/core/ext/filters/client_channel/resolver/binder/binder_resolver.cc Outdated Show resolved Hide resolved
src/core/ext/transport/binder/client/binder_connector.cc Outdated Show resolved Hide resolved
src/core/lib/address_utils/parse_address.cc Outdated Show resolved Hide resolved
src/core/lib/address_utils/sockaddr_utils.cc Outdated Show resolved Hide resolved
src/core/lib/event_engine/posix_engine/tcp_socket_utils.cc Outdated Show resolved Hide resolved
src/core/lib/iomgr/unix_sockets_posix.cc Outdated Show resolved Hide resolved
test/core/address_utils/sockaddr_utils_test.cc Outdated Show resolved Hide resolved
test/core/end2end/end2end_test_suites.cc Show resolved Hide resolved
test/core/end2end/end2end_test_suites.cc Outdated Show resolved Hide resolved
test/core/end2end/end2end_test_suites.cc
// }
// return std::string(buff.data(), len);
std::string temp_dir = "C:/tmp/";
if (CreateDirectoryA(temp_dir.c_str(), NULL) == 0 &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this implementation ends up working, we can create this directory once and return a static variable. That would let us eliminate the lambda captures below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution works. Changed to a const global variable. The directory is created only once now.
In future we might want to use a local directory for this test.

@drfloob drfloob added kokoro:force-run release notes: yes Indicates if PR needs to be in release notes and removed disposition/Needs Internal Changes labels Dec 19, 2023
@drfloob drfloob changed the title Support AF_UNIX for windows [EventEngine] Support AF_UNIX for windows Dec 19, 2023
drfloob
drfloob reviewed Jan 30, 2024
View reviewed changes
Copy link
Member

@drfloob drfloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry again for the delay. This is getting closer! I've done another review, please let me know if you have any questions.

src/core/lib/event_engine/windows/windows_engine.cc Outdated Show resolved Hide resolved
src/core/lib/event_engine/windows/windows_engine.cc Show resolved Hide resolved
src/core/lib/event_engine/windows/windows_engine.cc Outdated Show resolved Hide resolved
src/core/lib/event_engine/windows/windows_listener.cc Outdated Show resolved Hide resolved
src/core/lib/event_engine/windows/windows_listener.cc Outdated Show resolved Hide resolved
src/core/lib/event_engine/windows/windows_listener.cc Outdated Show resolved Hide resolved
include/grpc/support/port_platform.h Outdated Show resolved Hide resolved
src/core/lib/iomgr/tcp_client_windows.cc Outdated Show resolved Hide resolved
src/core/lib/iomgr/tcp_client_windows.cc Outdated Show resolved Hide resolved
src/core/lib/iomgr/tcp_client_windows.cc Outdated Show resolved Hide resolved
@youyuanwu
Copy link
Contributor Author

@drfloob The last test run had failures when event engine listener is turned off. I cannot see the test run links anymore and do not remember the config to turn on/off, so please rerun the pipeline again.
Let me know if that use case needs to be supported as well.

@dpservis
Copy link

dpservis commented Feb 5, 2024

I am updating this page 100 times a day, looking for the moment when the PR will be merged in production. This is a great achievement and I am looking forward to using it.

@drfloob
Copy link
Member

drfloob commented Feb 12, 2024

Status: experiment=-event_engine_listener is supported now.

That's great, thank you!

One more finding: This change might have exposed a separate potential deadlock bug in windows event engine client. With $env:GRPC_EXPERIMENTS="event_engine_client enabled. The client of uds will hit assert in debug mode about potential deadlock if server is not listening. This is related to grpc_core::ExecCtx dispatching pending tasks that use locks. Will create a spearate issue about this, since I do not think the current change is the culprit.

Very interesting! I'll look into that. Is there any test in particular that demonstrates the bug better than others?

@drfloob Can you rerun the ci?

It's running now.

@drfloob
Copy link
Member

drfloob commented Feb 12, 2024

I see the mutex re-acquisition problem https://source.cloud.google.com/results/invocations/419395f4-3a67-4bd1-9c73-f3fe2a43c1e5/targets/%2F%2Ftest%2Fcore%2Fend2end:connectivity_test@experiment%3Dpromise_based_client_call/log

@youyuanwu
Copy link
Contributor Author

I see the mutex re-acquisition problem https://source.cloud.google.com/results/invocations/419395f4-3a67-4bd1-9c73-f3fe2a43c1e5/targets/%2F%2Ftest%2Fcore%2Fend2end:connectivity_test@experiment%3Dpromise_based_client_call/log

Yes. This is the symptom. The easiest repro is to create any uds client that try connect to a non-existent server. Somewhere inside "event_engine_tcp_client_connect()" has the problem.

@drfloob drfloob mentioned this pull request Feb 13, 2024
Closed
@drfloob
Copy link
Member

drfloob commented Feb 13, 2024

@youyuanwu I have a fix in the linked PR. Thanks for helping us find some bugs!

copybara-service bot pushed a commit that referenced this pull request Feb 13, 2024
@drfloob
[EventEngine] Fix lock acquisition ordering problem on Windows Connect (
510dbc9 
#35892)

CC @youyuanwu

This fixes a lock acquisition ordering problem in WindowsEventEngine's Connect behavior, between a ConnectionState's internal mutex, and a HandshakeManager's mutex. The connection state mutex should not be held when calling the on_connected callback.

This should unblock #34801

Closes #35892

COPYBARA_INTEGRATE_REVIEW=#35892 from drfloob:connect-callback-lock-cycle 4d56120
PiperOrigin-RevId: 606752276
@youyuanwu
Copy link
Contributor Author

@drfloob I rebased with master with your change. The locking issue has been fixed by your change.

@drfloob
Copy link
Member

drfloob commented Feb 14, 2024

@youyuanwu Your change is merged, thanks for adding this feature!

@youyuanwu youyuanwu deleted the dev branch February 15, 2024 02:50
@youyuanwu
Copy link
Contributor Author

@drfloob Thanks for the review and help. I will leave the documentation change to you, to say that uds is no longer unix only.

@dpservis
Copy link

Hey, did this make it into main?

@drfloob
Copy link
Member

drfloob commented Feb 27, 2024

Hey, did this make it into main?

Yes. a58f83c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drfloob drfloob drfloob approved these changes

@Vignesh2208 Vignesh2208 Vignesh2208 approved these changes

@markdroth markdroth Awaiting requested review from markdroth markdroth is a code owner

Assignees

@drfloob drfloob

Labels
bloat/none lang/core per-call-memory/increase per-channel-memory/neutral PR title format release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@youyuanwu @drfloob @lostobject @dpservis @Vignesh2208 @grpc-kokoro @markdroth