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

Await activity creation to avoid deadlocks when running tests #9459

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gustavlrsn
Copy link
Member

No description provided.

Copy link
Member Author

gustavlrsn commented Nov 7, 2023

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @gustavlrsn and the rest of your teammates on Graphite Graphite

@gustavlrsn gustavlrsn mentioned this pull request Nov 7, 2023
@gustavlrsn gustavlrsn force-pushed the 11-07-Await_activity_creation_to_avoid_deadlocks branch from 9f744f0 to d993524 Compare November 8, 2023 11:32
@gustavlrsn gustavlrsn changed the title Await activity creation to avoid deadlocks Await activity creation to avoid deadlocks when running tests Nov 8, 2023
@gustavlrsn gustavlrsn marked this pull request as ready for review November 8, 2023 15:37
@gustavlrsn gustavlrsn force-pushed the 11-07-Await_activity_creation_to_avoid_deadlocks branch from d993524 to 568ec2a Compare November 8, 2023 15:40
@gustavlrsn gustavlrsn requested a review from znarf November 8, 2023 15:43
Copy link
Member

@Betree Betree left a comment

Choose a reason for hiding this comment

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

I like the idea of waiting for activities creation; I'm not a big fan of throwing Promises without looking at their result. We have many other places like that which would need to be updated if we want to make it a global change.

My main concern with the way it has been done here is that it actually changes the behavior in case there's a crash in the activity creation: we'll now return an error saying that the collective/event/fund/project creation failed, which is not true.

I'd recommend, inside generateCollectiveCreatedActivity, to wrap the activity creation call in a try ... catch, reporting the error to Sentry without re-throwing it if there's an issue.

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