Skip to content

Conversation

ramaraochavali
Copy link
Contributor

Signed-off-by: Rama rama.rao@salesforce.com

Description: To check for initiating call before continue decoding. See #4073 (comment) for more details to avoid the crash
Risk Level: Low
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Rama <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Contributor Author

@junr03 see if this helps the crash you are seeing. I am not fully familiar with network filter - so not sure if that condition check is required, but please review. If it does not, please feel free to revert the original commit.
cc : @ggreenway

Copy link
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Please add test coverage for this case.

Copy link
Member

@junr03 junr03 left a comment

Choose a reason for hiding this comment

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

No worries. Sorry, I forgot to update the original PR yesterday. Yes, I fixed at Lyft by doing what you are now proposing here.

As @ggreenway says, please add test coverage for this. Thanks!

@junr03
Copy link
Member

junr03 commented Aug 29, 2018

@ramaraochavali Lyft wants to get envoy to a clean state, so while we create tests here and get this merged we are going to revert. Here is the PR #4295. You'll just have to rebase master once that lands. Thanks!

@ggreenway
Copy link
Member

Merge conflicts.

The original change was backed out. Probably the easiest way to fix is to close this PR and create a new one, backout the backout, then apply this change on top of it.

@ramaraochavali
Copy link
Contributor Author

I will close this PR and open another one.

@ramaraochavali ramaraochavali deleted the fix/rate_limit_crash branch October 5, 2018 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants