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

openUri doesn't flush buffer commands on open (and why createConnection returns a Promise?) #5404

Closed
ValYouW opened this Issue Jun 26, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@ValYouW
Copy link
Contributor

ValYouW commented Jun 26, 2017

mongoose 4.11.0

When using openUri the connection onOpen is not getting called and as a result all collections are in "buffer" mode, maybe the following line should be replaced to _this.onOpen()
https://github.com/Automattic/mongoose/blob/master/lib/connection.js#L739

[Edit] I'll elaborate as I forgot to mention I did a little hack :)
What I was trying to achieve is make createConnection to always return the Connection object, whether it was called with useMongoClient or not. The fact that it returns a Promise is breaking my code as now I need to wait for the Promise to resolve before I can define models on this connection.
So maybe the correct question is "Why createConnection doesn't return the connection but a Promise?", since connection has a buffer mechanism, and it is an emitter that emits an "open" event it make sense to get it immediately (like it was before 4.11.0).

Thoughts?

@ValYouW ValYouW changed the title Shouldn't openUri call onOpen once connection is opened? Shouldn't openUri call onOpen once connection is opened? (or why createConnection returns a Promise?) Jun 27, 2017

@agoldis

This comment has been minimized.

Copy link

agoldis commented Jun 29, 2017

I have faced the issue when a query on a collection is executed before mongoose connects.

The code below never executes the find query:

const mongoose = require('mongoose')
mongoose.Promise = Promise
const schema = mongoose.Schema({}, {strict: false});
const Model = mongoose.model('mymodel', schema);

Model.find().exec().then(console.log)

mongoose.connect('mongodb://localhost:27017/testingBug', { useMongoClient: true })
.then(() => {
  console.log('connected')
})
.catch(console.error)

Here's what happens:

  1. connection is created using openUri (which is preferred method due to #5399)
  2. connection gets ready state
  3. it emits open event
  4. nothing really happens.

As @ValYouW suggests, calling _this.onOpen() solves the problem, because it triggers _this.collections[i].onOpen(); for every known collection, and sets collection's buffer to false

@ValYouW I suggest to change the title to something like openUri doesn't change collections buffer mode

UPDATE:
Here's a workaround

mongoose.connect('mongodb://localhost:27017/testingBug', { useMongoClient: true }, function (ignore, connection) {
  connection.onOpen()
}).then(() => {
  console.log('connected')
})
.catch(console.error)

@ValYouW ValYouW changed the title Shouldn't openUri call onOpen once connection is opened? (or why createConnection returns a Promise?) openUri doesn't flush buffer commands on open (and why createConnection returns a Promise?) Jun 30, 2017

@varunjayaraman

This comment has been minimized.

Copy link
Collaborator

varunjayaraman commented Jul 1, 2017

this seems like a legit enhancement to me, i'll add it to the docket. Thoughts @vkarpov15 ?

@varunjayaraman varunjayaraman added this to the 4.11.2 milestone Jul 1, 2017

@vkarpov15 vkarpov15 modified the milestones: 4.11.1, 4.11.2 Jul 2, 2017

@vkarpov15 vkarpov15 closed this in 6c025c8 Jul 2, 2017

@vkarpov15

This comment has been minimized.

Copy link
Collaborator

vkarpov15 commented Jul 2, 2017

You're right about the onOpen() part, fixed in 6c025c8. Thanks for reporting this issue and for trying out useMongoClient.

One of the changes for useMongoClient is that createConnection() and connect() return promises that resolve to the connection object if the initial connection succeeds. This is to make mongoose more consistent with the underlying driver and be more graceful with separating out initial connection errors (which are not recoverable) and connection errors that happen after initial collection (which may be recoverable). Right now we have a bunch of dirty hacks to support things like await mongoose.createConnection() and I think the promise-based direction is the right one going forward. If you have any concerns, please voice them here.

@ValYouW

This comment has been minimized.

Copy link
Contributor Author

ValYouW commented Jul 3, 2017

@vkarpov15 Thank you. Returning Promise from createConnection/connect doesn't give users the option to use the "command buffering" feature of mongoose (which is cool) on app startup (unless it is a single-connection and all operations are done on the default connection).
I will try to describe my scenario briefly: I'm developing an api server that works against 2 different databases (app/backoffice), I store both connections in some "connection manager" and on app startup all the models ask for their connection (either app/backoffice) and define the models on that connection, also on app startup, I have services that runs some queries against the different connections - this all happens on app startup without any need to wait for "ready" events etc, all thanks to the "command buffering" feature. Not being able to use this on startup will make my code a bit more complicated having to wait on connection open in all Models and services...
BTW the "connection manager" also takes core of any initial connection errors and it's not a problem at all...
Here is gist:
https://gist.github.com/ValYouW/b93798390551fa4b8bffaaed427b60cb

@vkarpov15

This comment has been minimized.

Copy link
Collaborator

vkarpov15 commented Jul 5, 2017

Yeah I can see that being frustrating. I suppose we can decorate connections with .then() if they're created with useMongoClient, so you can get the best of both worlds.

@vkarpov15 vkarpov15 reopened this Jul 5, 2017

@vkarpov15 vkarpov15 modified the milestones: 4.11.2, 4.11.1 Jul 5, 2017

@ValYouW

This comment has been minimized.

Copy link
Contributor Author

ValYouW commented Jul 5, 2017

@vkarpov15 thanks!

@tomgrossman

This comment has been minimized.

Copy link

tomgrossman commented Jul 6, 2017

@ValYouW I agree! I have the exact same issue. 3 different databases, so 3 different createConnection calls. Now with the useMongoClient I can't use these connections until the Promise is done. I expect a fix that will support command buffering feature which used to be amazing.

@vkarpov15 vkarpov15 closed this in 4495146 Jul 10, 2017

@vkarpov15

This comment has been minimized.

Copy link
Collaborator

vkarpov15 commented Jul 11, 2017

Fixes, will be released with 4.11.2

@tomgrossman

This comment has been minimized.

Copy link

tomgrossman commented Jul 25, 2017

@vkarpov15 can you please give an example how this solution helps? Because I can't still define a model for this connection since it is a promise and not a connection object.
Or at least add in the docs how to handle multiple DBs with the new useMongoClient option?

@vkarpov15

This comment has been minimized.

Copy link
Collaborator

vkarpov15 commented Jul 29, 2017

@tomgrossman you should be able to in 4.11.4. Is this not the case?

@tomgrossman

This comment has been minimized.

Copy link

tomgrossman commented Jul 31, 2017

@vkarpov15 seems to work, thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment