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.

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

Prevent HttpConsumer from hiding exceptions #1951

Merged
merged 1 commit into from
Dec 10, 2023

Conversation

adamchainz
Copy link
Member

@adamchainz adamchainz commented Nov 9, 2022

Fixes #1950.

The issue didn’t only affect tests—exceptions were always swallowed by converting them into StopConsumer(). So there may be failing HTTP consumers in production apps where the exceptions aren’t being logged.

@adamchainz
Copy link
Member Author

FYI @alexmclarty - if you recall, we spotted this issue whilst pairing (on top of the original one we were debugging).

@@ -81,7 +81,7 @@ async def http_request(self, message):
await self.handle(b"".join(self.body))
finally:
await self.disconnect()
raise StopConsumer()
raise StopConsumer()
Copy link
Member Author

Choose a reason for hiding this comment

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

what a difference an indentation level makes 😅

Copy link
Member

Choose a reason for hiding this comment

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

💝 — This may be the root of one I've been meaning to sit down with for an age.

I shall take a proper look later one.

Copy link
Member

Choose a reason for hiding this comment

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

So there may be failing HTTP consumers in production apps where the exceptions aren’t being logged.

Yes, that was the reported issue.

Copy link
Member

Choose a reason for hiding this comment

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

Little dance 🕺

Copy link
Member Author

Choose a reason for hiding this comment

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

🪩🕺

@th3hamm0r
Copy link

Thanks for fixing this well hidden bug! 👍
Any ETA for a release of that? It's easy to patch, so not a big problem for us.

@carltongibson
Copy link
Member

I'm planning new releases for the new year. This is on the roadmap for that.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Great. Thank you.

(Good job I didn't say which new year. 😳)

@carltongibson carltongibson merged commit e99ef8a into main Dec 10, 2023
@carltongibson carltongibson deleted the issue_1950_http_consumer_error_swallowing branch December 10, 2023 18:24
@adamchainz
Copy link
Member Author

Hehe no worries, thanks for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

HttpCommunicator does not raise exception from consumer
3 participants