Skip to content

Fix bug in url_helper/dual_stack() logging#1426

Merged
TheRealFalcon merged 5 commits into
canonical:mainfrom
holmanb:holmanb/url-helper-fix
May 11, 2022
Merged

Fix bug in url_helper/dual_stack() logging#1426
TheRealFalcon merged 5 commits into
canonical:mainfrom
holmanb:holmanb/url-helper-fix

Conversation

@holmanb
Copy link
Copy Markdown
Member

@holmanb holmanb commented May 3, 2022

Log all exceptions in dual_stack()

While debugging something that uses the dual_stack() function, I noticed that exceptions are not being logged as originally intended.

All exceptions thrown from futures in dual_stack() should be logged for debugging purposes.

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Looks good, but probably worth some unit testing checking exception behavior.

@holmanb holmanb force-pushed the holmanb/url-helper-fix branch 2 times, most recently from b03f21f to 6ac668c Compare May 11, 2022 02:53
@holmanb holmanb force-pushed the holmanb/url-helper-fix branch 2 times, most recently from a5c0a88 to 9ff55c5 Compare May 11, 2022 14:17
@holmanb holmanb force-pushed the holmanb/url-helper-fix branch from 9ff55c5 to db94e90 Compare May 11, 2022 14:24
@holmanb
Copy link
Copy Markdown
Member Author

holmanb commented May 11, 2022

@TheRealFalcon Assuming the last commit for black formatting, I think this is ready for re-review.

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

LGTM...and I think you had a little too much fun with the text in your tests 😄

@TheRealFalcon TheRealFalcon merged commit f0c457c into canonical:main May 11, 2022
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.

2 participants