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

Method call on server returns prematurely during mongo find/forEach unless fetch is chained #321

Closed
sra opened this Issue Sep 4, 2012 · 10 comments

Comments

Projects
None yet
3 participants
@sra

sra commented Sep 4, 2012

Details here: https://gist.github.com/112f1eda11939d16d197

When executing a method defined as follows:

Meteor.methods({
        'membercounts': function() {
            var housecounts = {};
            Houses.find().forEach(function(house){
                housecounts[house.name] = {count: Avatars.find({house:house.name, canvote:false, active:true}).count()};
            });
            return housecounts;
        }
});

The find() call seems to execute off in its own thread and the return returns an empty object or partially calculated depending on timing. If you add a .fetc() after the .find() it works as expected. The gist at the top of the issue has the output of the broken case.

@TomWij

This comment has been minimized.

Contributor

TomWij commented Sep 4, 2012

Since you aren't making this dependent on any data by the user, I don't think Meteor.method is the right way to go about this. In the current code you posted here, the client is simply asking the server for an array of documents.

Wait a minute...

  • An array of documents?! That's a collection.
  • Client asking for a collection?! That's subscribing.

Exactly, you could just publish the avatar counts of the houses and make the users subscribe to that.

Wait another minute...

We don't have the avatar counts in a houses in our collection, yet. So, you can add a count field to a house and whenever you add / remove an avatar you update that field. As a bonus, you can now query houses based on the amount of avatars they contain. It's a win-win situation.


I can imagine a situation where extra data stored in the document might not be the right approach, but I suppose that in this case it doesn't really matter. I feel like Meteor doesn't really support JOINs, but I might be wrong. However, I felt that trying to stay away from such constructs has usually resulted in much clearer code...

Take for example your piece of code and imagine an extremely high scalable setting, you're introducing an extra housecounts array (imposes more memory) and you're doing a find call for every found house (imposes a lot of processing); if you're going to support the amount of houses and people on our planet, this would surely be brought to its knee; storing one field extra in the house document, would not...

@sra

This comment has been minimized.

sra commented Sep 5, 2012

Having a find() unblock the current thread is still unexpected and potentially disastrous with respect to memory usage (because of the .fetch() requirement) - at least in my opinion. However I like the idea of maintaining a count on the house object for this particular case.

@glasser

This comment has been minimized.

Member

glasser commented Sep 5, 2012

Are you sure about this? Are you running this "Meteor.methods" call in the client or server? (ie, are you accidentally defining a stub rather than a server method?) On the server, forEach is intended to use Futures to block until the collection has been exhausted, so what you're reporting is a bug. Is "output.txt" printed by Node to your server console or in your browser?

@glasser

This comment has been minimized.

Member

glasser commented Sep 5, 2012

... although minimongo's forEach should be synchronous too.

What version of Meteor are you using when you see this?

(Tom's suggestions are good too.)

@sra

This comment has been minimized.

sra commented Sep 5, 2012

This is happening on the server, the method I pasted is in a file in the server/ folder and protected by Meteor.is_server for some reason. Meteor 0.4.0

The client is called with:

           Meteor.call("membercounts", function(err, data)
            {
                if (err)
                {
                    console.log(err);
                }
                .... stuff here
            });
@glasser

This comment has been minimized.

Member

glasser commented Sep 5, 2012

Two other suggestions:

  • Can you add a log in the callback BEFORE the "housecounts[house.name] = " line and tell me what is printed?
  • Ideally, can you create a minimal and complete Meteor app (not just a code snippet) that shows this problem?
@sra

This comment has been minimized.

sra commented Sep 5, 2012

Here you go: https://github.com/sra/meteor-issue321

Interestingly, the output of the line before that you asked for happens ALL before the return but the one after happens after the return. Spooky.

starting
loop top: {}
loop top: {}
loop top: {}
loop top: {}
returning
{}
loop bottom: {"0":{"count":250}}
loop bottom: {"0":{"count":250},"1":{"count":250}}
loop bottom: {"0":{"count":250},"1":{"count":250},"2":{"count":250}}
loop bottom: {"0":{"count":250},"1":{"count":250},"2":{"count":250},"3":{"count":250}}

@glasser

This comment has been minimized.

Member

glasser commented Sep 5, 2012

Thanks for the useful reproduction case! We spent the last hour playing with this over at Meteor HQ and figured out the issue; it's related to interactions between Fiber-based Meteor code and non-Fiber-based Mongo code. Should get a fix together after lunch today.

@TomWij

This comment has been minimized.

Contributor

TomWij commented Sep 5, 2012

It's not necessarily spooky, it's able to execute the console.log (loop top) for each item before returning. Whereas the next console.log (loop bottom) is stalled till after the return. If you were to execute this code not only once, but a ton of times; you could even notice a different order in the output of one of the runs. Although it's mostly going to give this output as forEach and return take some time to execute (flushig to console in involved, /me thinks).

@glasser glasser closed this Sep 5, 2012

@glasser

This comment has been minimized.

Member

glasser commented Sep 6, 2012

(This isn't pushed to devel (let alone master) yet but I guess that's how GitHub issue integration works.

glasser added a commit that referenced this issue Sep 6, 2012

Make Meteor.Cursor.forEach fully synchronous even if the user's callb…
…ack yields.

Previously, if the callback yields (eg, if it does a DB query), the next
iteration of the callback could run, or the forEach could finish entirely,
because the future we waited on only checked to ensure that the callback was
*started* on all documents, not *finished*.

Fixes #321. Thanks to Scott Anderson for bug report with minimal reproduction
case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment