Skip to content

[EventEngine] WindowsEventEngine Endpoint and Socket fixes#32385

Merged
drfloob merged 9 commits intogrpc:masterfrom
drfloob:ee/win-ee-endpoint-and-socket-fixes
Feb 15, 2023
Merged

[EventEngine] WindowsEventEngine Endpoint and Socket fixes#32385
drfloob merged 9 commits intogrpc:masterfrom
drfloob:ee/win-ee-endpoint-and-socket-fixes

Conversation

@drfloob
Copy link
Copy Markdown
Member

@drfloob drfloob commented Feb 14, 2023

A handful of problems were identified while writing the WindowsEventEngine Listener. To make the listener review easier, these fixes can be landed separately.

This is built upon #32376

Problems that are fixed in this PR:

  • OnConnectCompleted held a Mutex while calling the user callback, which can deadlock.
  • The WinSocket and some associated data needs to remain alive after the Endpoint destroyed, since Windows IOCP still needs to use some of that data. Endpoint destruction and socket shutdown are now decoupled, with the socket managed by a shared_ptr.

@drfloob drfloob marked this pull request as ready for review February 14, 2023 23:28
@drfloob drfloob requested a review from yashykt February 14, 2023 23:28
@drfloob drfloob merged commit a879544 into grpc:master Feb 15, 2023
yijiem added a commit that referenced this pull request Feb 16, 2023
ctiller added a commit that referenced this pull request Feb 16, 2023
…32419)

Reverts #32385

Co-authored-by: Yijie Ma <yijiem.main@gmail.com>
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Feb 16, 2023
drfloob added a commit that referenced this pull request Feb 22, 2023
…32440)

Relands #32385 (reverted in #32419) with fixes.

The Windows build is clean on a test cherrypick: cl/511291828

<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->

---------

Co-authored-by: drfloob <drfloob@users.noreply.github.com>
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
A handful of problems were identified while writing the
WindowsEventEngine Listener. To make the listener review easier, these
fixes can be landed separately.

This is built upon grpc#32376

Problems that are fixed in this PR:
* `OnConnectCompleted` held a Mutex while calling the user callback,
which can deadlock.
* The WinSocket and some associated data needs to remain alive after the
Endpoint destroyed, since Windows IOCP still needs to use some of that
data. Endpoint destruction and socket shutdown are now decoupled, with
the socket managed by a shared_ptr.

<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->

---------

Co-authored-by: drfloob <drfloob@users.noreply.github.com>
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
…rpc#32440)

Relands grpc#32385 (reverted in grpc#32419) with fixes.

The Windows build is clean on a test cherrypick: cl/511291828

<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->

---------

Co-authored-by: drfloob <drfloob@users.noreply.github.com>
wanlin31 pushed a commit that referenced this pull request May 18, 2023
A handful of problems were identified while writing the
WindowsEventEngine Listener. To make the listener review easier, these
fixes can be landed separately.

This is built upon #32376

Problems that are fixed in this PR:
* `OnConnectCompleted` held a Mutex while calling the user callback,
which can deadlock.
* The WinSocket and some associated data needs to remain alive after the
Endpoint destroyed, since Windows IOCP still needs to use some of that
data. Endpoint destruction and socket shutdown are now decoupled, with
the socket managed by a shared_ptr.

<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->

---------

Co-authored-by: drfloob <drfloob@users.noreply.github.com>
wanlin31 pushed a commit that referenced this pull request May 18, 2023
wanlin31 pushed a commit that referenced this pull request May 18, 2023
…32440)

Relands #32385 (reverted in #32419) with fixes.

The Windows build is clean on a test cherrypick: cl/511291828

<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->

---------

Co-authored-by: drfloob <drfloob@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bloat/none imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants