I find we might miss a return value of ListenerFilter::onAccept(ListenerFilterCallbacks& cb)
See the return type below.
enum class FilterStatus {
// Continue to further filters.
Continue,
// Stop executing further filters.
StopIteration
};
So it's impossible to express that onAccept find an error.
Currently if a listener filter wants to report an error, it must
- return StopIteration at onAccept
- register an event
- the callback of the above event call
cb.continueFilterChain
The drawback of this detour is that we have to report that error at the second event cycle.
What's worse, as long as it potentially report error, we have to wait another event cycle. See
|
Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) { |
|
ENVOY_LOG(debug, "tls inspector: new connection accepted"); |
|
Network::ConnectionSocket& socket = cb.socket(); |
|
ASSERT(file_event_ == nullptr); |
|
|
|
file_event_ = cb.dispatcher().createFileEvent( |
|
socket.ioHandle().fd(), |
|
[this](uint32_t events) { |
|
if (events & Event::FileReadyType::Closed) { |
|
config_->stats().connection_closed_.inc(); |
|
done(false); |
|
return; |
|
} |
|
|
|
ASSERT(events == Event::FileReadyType::Read); |
|
onRead(); |
|
}, |
|
Event::FileTriggerType::Edge, Event::FileReadyType::Read | Event::FileReadyType::Closed); |
|
|
|
cb_ = &cb; |
|
return Network::FilterStatus::StopIteration; |
|
} |
I am proposing
add a FilterStatus::Error
allow onAccept return it, and handle this value in ActiveSocket::continueFilterChain
Alternatively, since the FilterStatus type is also used by NetworkFilter, we can let onAccept return a new ListenerFilterStatus and only add Error here.
The benefits: we can save one epoll cycle on tls_inspector and http_inspector each, every time a connection is connected on listener which owns such filter.
@lizan @PiotrSikora thoughts?
I find we might miss a return value of
ListenerFilter::onAccept(ListenerFilterCallbacks& cb)See the return type below.
So it's impossible to express that onAccept find an error.
Currently if a listener filter wants to report an error, it must
cb.continueFilterChainThe drawback of this detour is that we have to report that error at the second event cycle.
What's worse, as long as it potentially report error, we have to wait another event cycle. See
envoy/source/extensions/filters/listener/tls_inspector/tls_inspector.cc
Lines 71 to 92 in b3c1306
I am proposing
add a FilterStatus::Error
allow onAccept return it, and handle this value in ActiveSocket::continueFilterChain
Alternatively, since the FilterStatus type is also used by NetworkFilter, we can let onAccept return a new
ListenerFilterStatusand only add Error here.The benefits: we can save one epoll cycle on tls_inspector and http_inspector each, every time a connection is connected on listener which owns such filter.
@lizan @PiotrSikora thoughts?