io: enable closed event for listener filter by soulxu · Pull Request #21585 · envoyproxy/envoy

soulxu · 2022-06-06T05:29:15Z

Signed-off-by: He Jie Xu hejie.xu@intel.com

Commit Message: io: enable the closed event for listener filter
Additional Description:
Previously the PR #18265 removed the Closed event for listener filter, since the Windows doesn't work well.

@ggreenway was found there is still have a corner case that can't be handled without the Closed event #18265 (comment)

This PR tries to introduce the Closed event back to the listener filter. As the comments on Windows code said, both Read and Closed can't be registered at same time due to the Connection doesn't support that yet. That means it shouldn't be a problem for the listener filter. Also currently, there is no Envoy code using both Read and Closed event, so it should be ok to remove those workaround code to make the both Read and Closed events registered to work with listener filter.

The full analysis is here #18265 (comment)

Risk Level: high
Testing: unittest should be added
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: Windows platform code changed.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only · 2022-06-06T05:29:19Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #21585 was opened by soulxu.

see: more, trace.

soulxu · 2022-06-06T05:29:38Z

/wait

Running some tests on the CI first.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-06-06T11:36:43Z

/retest

repokitteh-read-only · 2022-06-06T11:36:47Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21585 (comment) was created by @soulxu.

see: more, trace.

soulxu · 2022-06-06T14:01:59Z

cc @antoniovicente @wrowe

soulxu · 2022-06-06T14:05:44Z

source/common/event/file_event_impl.cc

-      if (events & FileReadyType::Closed && injected_activation_events_ & FileReadyType::Read) {
-        // We never ask for both early close and read at the same time. If close is requested
-        // keep that instead.
-        injected_activation_events_ = injected_activation_events_ & ~FileReadyType::Read;


I wasn't sure why the windows remove the read event, not sure there is the reason why windows can't handle both read and closed events. at least the CI works.

If nobody knows the history or background, a safer way is only register both read and closed events for non-Windows platforms, just silent ignore closed events when both read and closed events registered for Windows

@davinci26 do you remember the history of this?

I think it was an implementation detail based on how event registration worked at that point on Envoy and it wasnt a windows issue per se.

@rectified95 works now at Microsoft so he should be able to test event registrations and which events get raised.

soulxu · 2022-06-06T14:07:01Z

source/common/event/file_event_impl.cc

  ASSERT(dispatcher_.isThreadSafe());
  if (injected_activation_events_ != 0) {
-    // TODO(antoniovicente) remove this adjustment to activation events once ConnectionImpl can
-    // handle Read and Close events delivered together.


As this comment said, this code is for ConnectionImpl and can't work with both read and close events. So I assume both read and close event should work for listener filter.

soulxu · 2022-06-08T23:07:18Z

Also, note we don't have to fix this if we think this corner case doesn't worth taking the risk. Just give it a try if it looks workable.

yanavlasov · 2022-06-09T14:42:42Z

My understanding is that this issue is not deterministic and is just specific to Windows. If we do not have data that the Windows issue was addressed, then it seems to be unwise to revert this fix.

soulxu · 2022-06-10T01:52:22Z

My understanding is that this issue is not deterministic and is just specific to Windows. If we do not have data that the Windows issue was addressed, then it seems to be unwise to revert this fix.

The issue #18265 (comment) is deterministic. And this isn't specific for Windows.

The original PR #18265 is Windows-specific, without that PR, Windows will be stuck when the client sends the data slowly. Since the Windows will remove the Read event silently when both Read and Close events are registered.

After PR #18265 merged, we got the issue this PR is trying to fix. At least Linux has this issue.

@ggreenway and I have discussed about this, If the original fix makes the Windows works just except this corner case, it can be leave there. But still give a try, since I think the Windows workaround about slient remove Read event is for ConnectionImpl doesn't support both Read And Close event, so it should works for listener filter. (And today no any code register both Read and Close event, so those workaround never triggered), so try to fix it.

I'm thinking another safer way to fix is that just ignore the close event for Windows, but keep registering both read and close events for other platforms. Then we can fix this issue for other platforms, and keep untouch for Windows since wasn't very sure why Windows want to remove Read event sliently.

RyanTheOptimist · 2022-06-14T14:12:26Z

/assign @yanavlasov

RyanTheOptimist · 2022-06-14T14:12:33Z

/assign @ggreenway

ggreenway

Please add a test for the case that this fixes (client closes connection before listener filter has read enough data, but after it has written some bytes). I want to see if that test passes on windows in CI.

I'd also like to hear from @davinci26 or @wrowe or another windows expert to see if they recall the history of the behavior on windows.

/wait

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-06-17T08:44:20Z

Please add a test for the case that this fixes (client closes connection before listener filter has read enough data, but after it has written some bytes). I want to see if that test passes on windows in CI.

Thanks, done. I thought I have the test and close event handle in the existing code, but I'm wrong. Add them in the new commit.

soulxu · 2022-06-17T13:14:56Z

/retest

repokitteh-read-only · 2022-06-17T13:15:01Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21585 (comment) was created by @soulxu.

see: more, trace.

soulxu · 2022-06-18T00:14:26Z

Emm...it is a real failure on macos. @ggreenway do you know if this is a different behavior on macos? https://dev.azure.com/cncf/envoy/_build/results?buildId=110814&view=logs&j=cd7972b7-e5f6-5989-198f-f5f1ebdf95eb&t=e7ce6244-e6c5-5515-2efb-17f8c18c9157&l=13022

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ggreenway · 2022-06-22T16:48:13Z

@soulxu I looked through the libevent source. The feature we're wanting is EV_CLOSE, which can be checked with EV_FEATURE_EARLY_CLOSE. It appears that this is only supported with epoll, and MacOS uses kqueue.

soulxu · 2022-06-23T02:18:56Z

EV_FEATURE_EARLY_CLOSE

Thanks for the help! Let me add a check for EV_FEATURE_EARLY_CLOSE

ggreenway · 2022-06-23T17:18:16Z

I'm not sure if we want to start sprinkling libevent #defines in our codebase. It might be cleaner to just add #if for macos, with a comment of why it's not supported on this platform.

/wait

soulxu · 2022-06-24T02:39:01Z

I'm not sure if we want to start sprinkling libevent #defines in our codebase. It might be cleaner to just add #if for macos, with a comment of why it's not supported on this platform.

/wait

got it, we have the unittest to check it is support or not when an new platform added (although it is rare case), so it should be fine.

Also note, the EV_FEATURE_EARLY_CLOSE is checked by runtime https://libevent.org/doc/event_8h.html#aced890aa77dac1669325f74a27584f03, but yes, it is still sprinkling libevent.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-06-24T03:05:10Z

source/common/network/listener_filter_buffer_impl.cc

+// `FileReadyType::Closed` is required `EV_FEATURE_EARLY_CLOSE` feature for libevent, and this feature is
+// only supported with `epoll`. But `MacOS` uses the `kqueue`.
+// https://libevent.org/doc/event_8h.html#a98f643f9c9063a4cbf410f519eb61e55
+#if defined(__APPLE__)


@ggreenway I'm a little hesitant to add this check, even if we add Closed event for macOS, it won't break anything. maybe without this check will make the code clear? We can just skip the test for macOS. but let me know your opinion.

I think the check can be moved to the test only. Setting the Closed event doesn't cause any harm on macos I don't think; it just doesn't do anything.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ggreenway

/wait

ggreenway · 2022-06-29T14:40:23Z

source/common/network/listener_filter_buffer_impl.cc

+// `FileReadyType::Closed` is required `EV_FEATURE_EARLY_CLOSE` feature for libevent, and this feature is
+// only supported with `epoll`. But `MacOS` uses the `kqueue`.
+// https://libevent.org/doc/event_8h.html#a98f643f9c9063a4cbf410f519eb61e55
+#if defined(__APPLE__)


I think the check can be moved to the test only. Setting the Closed event doesn't cause any harm on macos I don't think; it just doesn't do anything.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

io: enable closed event for listener

7d7c0e3

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot added the waiting label Jun 6, 2022

fix test

fc0c166

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jun 6, 2022

soulxu changed the title ~~[WIP] io: enable closed event for listener filter~~ io: enable closed event for listener filter Jun 6, 2022

soulxu marked this pull request as ready for review June 6, 2022 14:01

soulxu requested review from KBaichoo, ggreenway and lizan as code owners June 6, 2022 14:01

soulxu commented Jun 6, 2022

View reviewed changes

repokitteh-read-only bot assigned yanavlasov Jun 14, 2022

repokitteh-read-only bot assigned ggreenway Jun 14, 2022

ggreenway requested changes Jun 16, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 16, 2022

Add test and handle close event

69587bc

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jun 17, 2022

improve the test

caa1f93

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot added the waiting label Jun 23, 2022

soulxu added 2 commits June 24, 2022 02:58

Add check for Apple

a2fa342

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

improve commit

fc21a6e

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jun 24, 2022

soulxu commented Jun 24, 2022

View reviewed changes

fix format

172f1b6

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

ggreenway requested changes Jun 29, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 29, 2022

soulxu added 2 commits June 29, 2022 15:14

Merge branch 'main' into introduce_close_event_back

71a41e2

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

address comment

6481fa0

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Jun 29, 2022

ggreenway approved these changes Jun 29, 2022

View reviewed changes

ggreenway enabled auto-merge (squash) June 29, 2022 15:42

ggreenway merged commit fb4567f into envoyproxy:main Jun 29, 2022

Conversation

soulxu commented Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jun 6, 2022

Uh oh!

soulxu commented Jun 6, 2022

Uh oh!

soulxu commented Jun 6, 2022

Uh oh!

repokitteh-read-only bot commented Jun 6, 2022

Uh oh!

soulxu commented Jun 6, 2022

Uh oh!

soulxu Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

davinci26 Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanavlasov commented Jun 9, 2022

Uh oh!

soulxu commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RyanTheOptimist commented Jun 14, 2022

Uh oh!

RyanTheOptimist commented Jun 14, 2022

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

soulxu commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soulxu commented Jun 17, 2022

Uh oh!

repokitteh-read-only bot commented Jun 17, 2022

Uh oh!

soulxu commented Jun 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggreenway commented Jun 22, 2022

Uh oh!

soulxu commented Jun 23, 2022

Uh oh!

ggreenway commented Jun 23, 2022

Uh oh!

soulxu commented Jun 24, 2022

Uh oh!

soulxu Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

ggreenway Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

soulxu Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

soulxu commented Jun 6, 2022 •

edited

Loading

soulxu Jun 6, 2022 •

edited

Loading

davinci26 Jun 17, 2022 •

edited

Loading

soulxu Jun 6, 2022 •

edited

Loading

soulxu commented Jun 8, 2022 •

edited

Loading

soulxu commented Jun 10, 2022 •

edited

Loading

soulxu commented Jun 17, 2022 •

edited

Loading

soulxu commented Jun 18, 2022 •

edited

Loading