Skip to content

ext_proc: Ensure that timer is always cancelled#17569

Merged
snowp merged 5 commits intoenvoyproxy:mainfrom
gbrail:fix-timer-cancellation
Aug 9, 2021
Merged

ext_proc: Ensure that timer is always cancelled#17569
snowp merged 5 commits intoenvoyproxy:mainfrom
gbrail:fix-timer-cancellation

Conversation

@gbrail
Copy link
Contributor

@gbrail gbrail commented Aug 3, 2021

Commit Message: Ensure that the timer is cancelled when an "ImmediateResponse"
message is received.
Additional Description:
Risk Level: Low
Testing: New unit tests
Docs Changes:
Release Notes:
Platform Specific Features:
Signed-off-by: Gregory Brail gregbrail@google.com

@gbrail
ext_proc: Ensure that timer is always cancelled
1993922 
Ensure that the timer is cancelled when an "ImmediateResponse"
message is received.

Signed-off-by: Gregory Brail <gregbrail@google.com>
@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Aug 3, 2021

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: #17569 was opened by gbrail.

see: more, trace.

@gbrail gbrail marked this pull request as ready for review August 3, 2021 06:50
@gbrail gbrail requested a review from snowp as a code owner August 3, 2021 06:50
@gbrail
Copy link
Contributor Author

gbrail commented Aug 3, 2021

Presubmit tests are failing because some other modules have fallen below the coverage threshold -- the ext_proc code has fine coverage.

@gbrail
ext_proc: Add one more extra check
dfd901d 
Signed-off-by: Gregory Brail <gregbrail@google.com>
snowp
snowp suggested changes Aug 3, 2021
View reviewed changes
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks, a couple comments but I think this makes sense

@snowp snowp self-assigned this Aug 3, 2021
@snowp snowp added the waiting label Aug 3, 2021
cboitel
cboitel reviewed Aug 3, 2021
View reviewed changes
Copy link

@cboitel cboitel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In other onXXX, test to check process_complete_ is performed after stats are updated not before.

In onGrpcClose, returning before stat update would leave the metric unchanged if external processor asks to send an immediate response thus rendering the metric less accurate.

gbrail added 2 commits August 4, 2021 17:43
@gbrail
Merge branch 'main' into fix-timer-cancellation
bb816cc 
Signed-off-by: Gregory Brail <gregbrail@google.com>
@gbrail
ext_proc: Address code review comments
74e5f79 
Signed-off-by: Gregory Brail <gregbrail@google.com>
snowp
snowp previously approved these changes Aug 5, 2021
View reviewed changes
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks!

/retest

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Aug 5, 2021

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

🐱

Caused by: a #17569 (review) was submitted by @snowp.

see: more, trace.

@snowp
Copy link
Contributor

snowp commented Aug 5, 2021

Looks like a timeout in one of the ext_proc tests, can you take a look? Not sure if its this PR or if the test is just flaky

@gbrail
ext_proc: streaming_integration_test is medium with tsan
a7dab57 
Signed-off-by: Gregory Brail <gregbrail@google.com>
@gbrail
Copy link
Contributor Author

gbrail commented Aug 5, 2021

I was trying to be efficient and changed the size of that test to "small" but since I see it timing out in 60 seconds it's clearly "medium" at least in the tsan case.

snowp
snowp approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks!

@snowp snowp merged commit 7da96ad into envoyproxy:main Aug 9, 2021
@cboitel
Copy link

cboitel commented Aug 10, 2021

Is there any chance to have this change in one of the v1.19 maintenance release ?

mpuncel added a commit to mpuncel/envoy that referenced this pull request Aug 11, 2021
@mpuncel
Merge branch 'main' into mpuncel/sds-hc-sequence
5b140de 
* main: (687 commits)
  ci: set build debug information from env (envoyproxy#17635)
  ext_authz: do the authentication even the direct response is set (envoyproxy#17546)
  upstream: various cleanups in connection pool code (envoyproxy#17644)
  owners: promote Dmitry to maintainer (envoyproxy#17642)
  quiche: client session supports creating bidi stream (envoyproxy#17543)
  Update HTTP/2 METADATA documentation. (envoyproxy#17637)
  ext_proc: Check validity of the :status header (envoyproxy#17596)
  test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614)
  ensure that the inline cookie header will be folded correctly  (envoyproxy#17560)
  cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616)
  owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641)
  quiche: update QUICHE dependency (envoyproxy#17618)
  Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623)
  REPO_LAYOUT.md: fix outdated link (envoyproxy#17626)
  hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558)
  thrift proxy: add request shadowing support (envoyproxy#17544)
  ext_proc: Ensure that timer is always cancelled (envoyproxy#17569)
  Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362)
  proto: fix verify to point at v3 only (envoyproxy#17622)
  api: move generic matcher proto to its own package (envoyproxy#17096)
  ...
mpuncel added a commit to mpuncel/envoy that referenced this pull request Aug 16, 2021
@mpuncel
Merge branch 'main' into mpuncel/sds-hc-sequence
dab7720 
* main: (687 commits)
  ci: set build debug information from env (envoyproxy#17635)
  ext_authz: do the authentication even the direct response is set (envoyproxy#17546)
  upstream: various cleanups in connection pool code (envoyproxy#17644)
  owners: promote Dmitry to maintainer (envoyproxy#17642)
  quiche: client session supports creating bidi stream (envoyproxy#17543)
  Update HTTP/2 METADATA documentation. (envoyproxy#17637)
  ext_proc: Check validity of the :status header (envoyproxy#17596)
  test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614)
  ensure that the inline cookie header will be folded correctly  (envoyproxy#17560)
  cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616)
  owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641)
  quiche: update QUICHE dependency (envoyproxy#17618)
  Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623)
  REPO_LAYOUT.md: fix outdated link (envoyproxy#17626)
  hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558)
  thrift proxy: add request shadowing support (envoyproxy#17544)
  ext_proc: Ensure that timer is always cancelled (envoyproxy#17569)
  Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362)
  proto: fix verify to point at v3 only (envoyproxy#17622)
  api: move generic matcher proto to its own package (envoyproxy#17096)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
@gbrail gbrail deleted the fix-timer-cancellation branch September 23, 2021 18:49
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
@gbrail
ext_proc: Ensure that timer is always cancelled (envoyproxy#17569)
d261596 
Signed-off-by: Gregory Brail <gregbrail@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@cboitel cboitel cboitel left review comments

@snowp snowp snowp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@snowp snowp

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gbrail @snowp @cboitel