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

Rewrite find_offers() so that CancelledError is not caught #308

Merged
merged 1 commit into from
Apr 2, 2021

Conversation

azawlocki
Copy link
Contributor

@azawlocki azawlocki commented Apr 1, 2021

In Python up to 3.7, asyncio.CancelledError is a subclass of Exception, so one has to be careful in order not to intercept CancelledError with an except Exception clause.

Fixes golemfactory/yagna-triage#101

@azawlocki azawlocki requested a review from a team April 1, 2021 16:04
@azawlocki azawlocki changed the base branch from master to b0.5 April 1, 2021 16:04
@azawlocki azawlocki requested a review from filipgolem April 1, 2021 16:05
)
async def reject_proposal(proposal, reason):
await proposal.reject(reason=reason)
emit(events.ProposalRejected(prop_id=proposal.id, reason=reason))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's safer not to catch any exceptions here and let them bubble up

Comment on lines -418 to -420
except InvalidPropertiesError as err:
await reject_proposal(proposal, "Malformed offer")
continue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error will be handled by except Exception at the end of find_offers().

@azawlocki azawlocki force-pushed the az/find-offers-cancellation branch from 7e1eba2 to eff6468 Compare April 2, 2021 15:26
@azawlocki azawlocki merged commit ceba412 into b0.5 Apr 2, 2021
@azawlocki azawlocki deleted the az/find-offers-cancellation branch April 2, 2021 15:36
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