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

Ensure every instance of StreamFilter and PassThroughFilter to contain only one copy of StreamFilterBase. #35341

Merged
merged 2 commits into from
Aug 1, 2024
Merged

Ensure every instance of StreamFilter and PassThroughFilter to contain only one copy of StreamFilterBase. #35341

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9dd0af
Change Stream(Decode|Encode)Filter to inherit from StreamFilterBase
wu-bin Jul 22, 2024
359cb8f
Replace a default implementation of StreamFilterBase::onDestroy by 3
wu-bin Jul 31, 2024
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
6 changes: 3 additions & 3 deletions envoy/http/filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,7 @@ class StreamFilterBase {
* onDestroy() invoked. Filters that cross-register as access log handlers receive log() before
* onDestroy().
*/
virtual void onDestroy() PURE;
virtual void onDestroy() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you keep this method pure virtual if you add override onDestroy in the PassThroughFilter? Or is it causing more build errors than that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Yan, but I think if we do that, an existing class will not compile if

  • It derives directly from PassThroughDecoderFilter or PassThroughEncoderFilter, and
  • It doesn't override onDestroy, i.e. it relies on the empty implementation in PassThroughDecoderFilter or PassThroughEncoderFilter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/assign @yanavlasov

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try it? Can you post the compiler error, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, I change to have 3 onDestroy implementations in PassThroughDecoderFilter, PassThroughEncoderFilter, and PassThroughFilter. PTAL.


/**
* Called when a match result occurs that isn't handled by the filter manager.
Expand Down Expand Up @@ -901,7 +901,7 @@ class StreamFilterBase {
/**
* Stream decoder filter interface.
*/
class StreamDecoderFilter : public StreamFilterBase {
class StreamDecoderFilter : public virtual StreamFilterBase {
public:
/**
* Called with decoded headers, optionally indicating end of stream.
Expand Down Expand Up @@ -1117,7 +1117,7 @@ class StreamEncoderFilterCallbacks : public virtual StreamFilterCallbacks {
/**
* Stream encoder filter interface.
*/
class StreamEncoderFilter : public StreamFilterBase {
class StreamEncoderFilter : public virtual StreamFilterBase {
public:
/**
* Called with supported 1xx headers.
Expand Down
6 changes: 0 additions & 6 deletions source/extensions/filters/http/common/pass_through_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@ namespace Http {
// A decoder filter which passes all data through with Continue status.
class PassThroughDecoderFilter : public virtual StreamDecoderFilter {
public:
// Http::StreamFilterBase
void onDestroy() override {}

// Http::StreamDecoderFilter
Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool) override {
return Http::FilterHeadersStatus::Continue;
Expand All @@ -32,9 +29,6 @@ class PassThroughDecoderFilter : public virtual StreamDecoderFilter {
// An encoder filter which passes all data through with Continue status.
class PassThroughEncoderFilter : public virtual StreamEncoderFilter {
public:
// Http::StreamFilterBase
void onDestroy() override {}

// Http::StreamEncoderFilter
Http::Filter1xxHeadersStatus encode1xxHeaders(Http::ResponseHeaderMap&) override {
return Http::Filter1xxHeadersStatus::Continue;
Expand Down
Loading