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

event: assert the case of both read and closed event registered #18265

Merged
merged 10 commits into from
Oct 27, 2021
Merged

event: assert the case of both read and closed event registered #18265

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2230a06
Assert the case of both read and closed event registered
soulxu Sep 26, 2021
6d769f1
Remove the closed event for http inspect
soulxu Sep 26, 2021
e931f3d
Fix the test
soulxu Oct 11, 2021
488936f
fix test
soulxu Oct 11, 2021
ae5004f
update coverage
soulxu Oct 11, 2021
98774b7
address comment
soulxu Oct 12, 2021
1fe505c
skip failed test for windows
soulxu Oct 13, 2021
481a41c
update coverage
soulxu Oct 13, 2021
ca33eef
address comment
soulxu Oct 14, 2021
308ffbb
Add event tunning back
soulxu Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions source/common/event/file_event_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,8 @@ void FileEventImpl::registerEventIfEmulatedEdge(uint32_t event) {
ASSERT((event & (FileReadyType::Read | FileReadyType::Write)) == event);
if (trigger_ == FileTriggerType::EmulatedEdge) {
auto new_event_mask = enabled_events_ | event;
if (event & FileReadyType::Read && (enabled_events_ & FileReadyType::Closed)) {
// We never ask for both early close and read at the same time.
new_event_mask = new_event_mask & ~FileReadyType::Read;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem safe to me to remove this code. This is a change in behavior in cases where the ASSERT above about not allowing read and closed at the same time fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

got it, let me add it back.

// We never ask for both early close and read at the same time.
Copy link
Contributor

Choose a reason for hiding this comment

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

@davinci26 Can you confirm that libevent on Windows supports EV_CLOSE?

Copy link
Member

Choose a reason for hiding this comment

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

Does EV_CLOSE correspond to EPOLLRDHUP? if yes then it is supported on wepoll

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like it. I guess Windows and Linux support this, but not Mac which uses the kqueue for event handling in libevent.

ASSERT(!(event & FileReadyType::Read && (enabled_events_ & FileReadyType::Closed)));
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to ASSERT against new_event_mask instead.

updateEvents(new_event_mask);
}
}
Expand All @@ -150,11 +148,10 @@ void FileEventImpl::mergeInjectedEventsAndRunCb(uint32_t events) {
// TODO(antoniovicente) remove this adjustment to activation events once ConnectionImpl can
// handle Read and Close events delivered together.
if constexpr (PlatformDefaultTriggerType == FileTriggerType::EmulatedEdge) {
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

See above, this is a change in behavior. Adding an ASSERT is not a guarantee that these cases won't come up while running a real proxy.

Also note that injected_activation_events_ may contain events that the connection is not registered for. I don't think it happens often but it is technically allowed.

}
// We never ask for both early close and read at the same time. If close is requested
// keep that instead.
ASSERT(
!(events & FileReadyType::Closed && injected_activation_events_ & FileReadyType::Read));
}

events |= injected_activation_events_;
Expand Down
14 changes: 6 additions & 8 deletions source/extensions/filters/listener/tls_inspector/tls_inspector.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,6 @@ Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) {
socket.ioHandle().initializeFileEvent(
cb.dispatcher(),
[this](uint32_t events) {
if (events & Event::FileReadyType::Closed) {
config_->stats().connection_closed_.inc();
done(false);
return;
}

ASSERT(events == Event::FileReadyType::Read);
ParseState parse_state = onRead();
switch (parse_state) {
Expand All @@ -113,8 +107,7 @@ Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) {
break;
}
},
Event::PlatformDefaultTriggerType,
Event::FileReadyType::Read | Event::FileReadyType::Closed);
Event::PlatformDefaultTriggerType, Event::FileReadyType::Read);
return Network::FilterStatus::StopIteration;
}
NOT_REACHED_GCOVR_EXCL_LINE;
Expand Down Expand Up @@ -176,6 +169,11 @@ ParseState Filter::onRead() {
return ParseState::Error;
}

if (result.return_value_ == 0) {
config_->stats().connection_closed_.inc();
return ParseState::Error;
}

// Because we're doing a MSG_PEEK, data we've seen before gets returned every time, so
// skip over what we've already processed.
if (static_cast<uint64_t>(result.return_value_) > read_) {
Expand Down