Skip to content

grid: handle immediate success/failure#15892

Merged
alyssawilk merged 2 commits intoenvoyproxy:mainfrom
alyssawilk:reentrant
Apr 8, 2021
Merged

grid: handle immediate success/failure#15892
alyssawilk merged 2 commits intoenvoyproxy:mainfrom
alyssawilk:reentrant

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk commented Apr 8, 2021

Handle when newStream immediately connects/fails. In this case, all the wrappers should be immediately deleted, and return nullptr rather than returning wrappers.

Risk Level: n/a (code unused)
Testing: new unit tests
Docs Changes: n/a
Release Notes: n/a

Part of #15649

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
DavidSchinazi
DavidSchinazi previously approved these changes Apr 8, 2021
Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi left a comment

Choose a reason for hiding this comment

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

Nice!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
ConnectionAttemptCallbacks(WrapperCallbacks& parent, PoolIterator it);

// Returns true if a stream is immediately created, false if it is pending.
bool newStream();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be more readable to have an enum instead of a bool here. But you can leave it like this if you like it better. I got confused trying to read the code before seeing this comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good - will do that in a follow-up!

@alyssawilk alyssawilk merged commit 0abd955 into envoyproxy:main Apr 8, 2021
@alyssawilk alyssawilk deleted the reentrant branch February 28, 2022 21:25
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