Publications are ready before object merges are complete since 1.8.2

[jrkinclus]

Describe the bug Publications' ready() value doesn't correspond to when objects are actually available. Especially when the publication is loading a lot of data and takes multiple seconds to finish.

The behaviour I'm seeing in our app is that one object that is published by two publishComposite publications but with different fields does not get merged at the time when both of the large and slow publication s report that sub.ready() === true.

Seems that this line: https://github.com/Meteor-Community-Packages/meteor-publish-composite/blob/master/lib/publish_composite.js#L24 Should in fact be await this.ready(); Nope, doesn't help. Something else is the problem.

To Reproduce Reproduction test repo: https://github.com/jrkinclus/simple-todos-react My screencasts showing the behaviour: https://github.com/Meteor-Community-Packages/meteor-publish-composite/issues/163#issuecomment-1801850510

Expected behavior Objects published with publishComposite should be finalized and merged at the point in time when sub.ready() === true

[github-actions[bot]]

Thank you for submitting this issue!

We, the Members of Meteor Community Packages take every issue seriously. Our goal is to provide long-term lifecycles for packages and keep up with the newest changes in Meteor and the overall NodeJs/JavaScript ecosystem.

However, we contribute to these packages mostly in our free time. Therefore, we can't guarantee your issues to be solved within certain time.

If you think this issue is trivial to solve, don't hesitate to submit a pull request, too! We will accompany you in the process with reviews and hints on how to get development set up.

Please also consider sponsoring the maintainers of the package. If you don't know who is currently maintaining this package, just leave a comment and we'll let you know

[StorytellerCZ]

Looking at this, I think we will have to re-visit that thing and probably revert for now.

[StorytellerCZ]

Can you please check v1.8.4 to see if it still persist there?

[jrkinclus]

Yes, still happening in 1.8.4

[StorytellerCZ]

Thank you for the feedback. Will continue investigating.

[jrkinclus]

I finally got to making a test repo and tested this thorougly. https://github.com/jrkinclus/simple-todos-react

I was wrong about the await fix, it doesn't help at all.

Here are screencasts which illustrate the problem of subscriptions being ready too soon: publish-composite-1.8.4_bad.webm publish-composite-1.8.3_bad.webm publish-composite-1.8.2_bad.webm publish-composite-1.8.1_ok.webm publish-composite-1.8.0_ok.webm

In 1.8.0 and 1.8.1 you see that the subscription handles are only ready once the entire massive query for all the Tasks has finished. Starting from 1.8.2, they return ready before the queries are done. The consequence of this is that the the same user object, which is published in both publications but with different fields, doesn't reach the point of having all the fields published until the entire query finishes. This seems to specifically be a problem in the merging of objects that are published twice.

In our React app, we show a loading page until subscriptions return ready and then attempt to display the objects returned from useTracker calls. In 1.8.2+ due to the loading page going away too soon, our app attempts to display certain objects that are being published by two publications in a similar way as in the simple-todos-react I created. This leads to blank content and broken UI views as fields that previously were expected to be available aren't actually yet available.

[manueltimita]

In 1.8.2, the parents .find and children have been converted to async/await. Prior to this, given the "synchronous" code allowed by Fibers, this.ready() in publish_composite.js was called last, after all added callbacks in the observer have run once.

This bit from the Meteor docs is relevant:

"Before observe returns, added (or addedAt) will be called zero or more times to deliver the initial results of the query."

Looks like in 1.8.2 and onwards, the code below does not guarantee that this.ready() is called only after the added callback has run for all children in this.observeHandle in publication.js:

function publishComposite (name, options) {
  return Meteor.publish(name, async function publish (...args) {
    const subscription = new Subscription(this)
    const instanceOptions = await prepareOptions.call(this, options, args)
    const publications = []

    for (const opt of instanceOptions) {
      const pub = new Publication(subscription, opt)
      await pub.publish()
      publications.push(pub)
    }

    this.onStop(() => {
      publications.forEach(pub => pub.unpublish())
    })

    // here we need some code to wait for all publications to finish processing initial added callbacks
    // ...

    debugLog('Meteor.publish', 'ready')
    this.ready()
  })
}

To ensure that, we need to track when all initial documents have been processed by the added callback. I would prepare a pull request, but I have no idea yet of the full impact - in my cases it works, but maybe under workload something else will break. I need more time to see it at work in production.

So, here is a quick fix, if you are willing to test it in your use case. The files concerned are publish_composite.js and publish.js.

Add this property to the constructor in publication.js:

    // property to store promises for added callbacks
    this.addedPromises = [];

Replace the added callback of the observer in publish(), in file publish.js, with this:

      added: Meteor.bindEnvironment(async (doc) => {
        const addedPromise = new Promise(async (resolve) => {
          // ... th rest of the callback code

          // resolve the added promise at the end of the added callback
          resolve();
        });

        // store the promise
        this.addedPromises.push(addedPromise);
      })

Replace publishComposite() in publish_composite.js with this:

function publishComposite (name, options) {
  return Meteor.publish(name, async function publish (...args) {
    // ... the rest of the code

    // wait for all publications to finish processing initial added callbacks
    await Promise.all(publications.flatMap(pub => pub.addedPromises));

    debugLog('Meteor.publish', 'ready')
    this.ready()
  })
}

You should see everything working as before. Feedback would be very welcome.

[jrkinclus]

I'm uncertain that I'm understanding what was supposed to be left in the parts you omitted as just "... the rest of the code". Did you mean that the whole existing added call should be wrapped in the in the new Promise( like this:

added: Meteor.bindEnvironment(async (doc) => {
  const addedPromise = new Promise(async (resolve) => {
    const alreadyPublished = this.publishedDocs.has(doc._id)

    if (alreadyPublished) {
      debugLog('Publication.observeHandle.added', `${collectionName}:${doc._id} already published`)
      this.publishedDocs.unflagForRemoval(doc._id)
      this._republishChildrenOf(doc)
      this.subscription.changed(collectionName, doc._id, doc)
    } else {
      this.publishedDocs.add(collectionName, doc._id)
      await this._publishChildrenOf(doc)
      this.subscription.added(collectionName, doc)
    }
    // resolve the added promise at the end of the added callback
    resolve();
  });

  // store the promise
  this.addedPromises.push(addedPromise);
}),

And for publishComposite, adding the await Promise.all at the end like so:

function publishComposite(name, options) {
  return Meteor.publish(name, async function publish(...args) {
    const subscription = new Subscription(this)
    const instanceOptions = await prepareOptions.call(this, options, args)
    const publications = []

    for (const opt of instanceOptions) {
      const pub = new Publication(subscription, opt)
      await pub.publish()
      publications.push(pub)
    }

    this.onStop(() => {
      publications.forEach(pub => pub.unpublish())
    })
    // wait for all publications to finish processing initial added callbacks
    await Promise.all(publications.flatMap(pub => pub.addedPromises));

    debugLog('Meteor.publish', 'ready')
    await this.ready()
  })
}

I'm testing this in the simple-todos-react repo I made and does it seem to be fixing the issue. I edited v1.8.4 of publish-composite. The two publications became ready at the the end simultaenously with the merging of the user object:

Screencast from 06-12-23 18:24:11.webm

[manueltimita]

Sounds like good news if it works for you too!

Did you mean that the whole existing added call should be wrapped in the in the new Promise( like this

Yes, judging by the code you pasted.

[StorytellerCZ]

@manueltimita please prepare a PR already, we can do a RC release so that people can test is easily.

[manueltimita]

@jrkinclus could you try reywood:publish-composite@1.8.5-beta.2 and let us know if it works as expected