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

Meteor 3.0 - FIX - publication handler is not being waited on #12972

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

renanccastro
Copy link
Contributor

Hello!
Found this issue while seeing metrics on Monti APM with Meteor 3.

When we have an async publication handler, Meteor livedata server is not waiting for the promise resolution(either success/error) to unblock the processMessage handler.

The current behavior is like all publications are being unblocked.

This PR fixes it.

…essage in queue (except when unblocked explicitily)
@renanccastro renanccastro reopened this Jan 18, 2024
@renanccastro renanccastro changed the base branch from release-3.0-2.14 to release-3.0 January 18, 2024 14:09
Copy link
Collaborator

@filipenevola filipenevola left a comment

Choose a reason for hiding this comment

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

Nice catch.

Copy link
Contributor

@denihs denihs left a comment

Choose a reason for hiding this comment

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

Nice! Once the tests succeed, we can merge this.

@filipenevola
Copy link
Collaborator

@denihs I think at least one test should be added to avoid regressions for this case, right? cc @renanccastro

@nachocodoner
Copy link
Member

Nice! Once the tests succeed, we can merge this.

Just mentioning here that the test that fails here is the same that fails from the release-3.0 base branch, so it is not related with these changes. It also happens on a branch that I am fixing an issue for 3.x. There should be a branch that was merged against 3.0 that introduced it.

release-3.0 branch failure:
https://github.com/meteor/meteor/pull/12359/checks?check_run_id=20620785033

@renanccastro
Copy link
Contributor Author

@denihs added a new regression test for this case

@StorytellerCZ StorytellerCZ added this to the Release 3.0 milestone Jan 19, 2024
@denihs
Copy link
Contributor

denihs commented Jan 23, 2024

Merging it since the test failing here is already solved in the 3.0 branch

@denihs denihs merged commit 40785cb into meteor:release-3.0 Jan 23, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants