Domain module support #1337

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
@DrewMartin

Added domain support to mongoose.

Almost every query uses either a Promise or utils.tick so both of those will bind the current process's domain to the callback, if the process has a domain. As far as I could tell, the only other places that needed it were mapReduce and aggregate.

@aheckmann

This comment has been minimized.

Show comment
Hide comment
@aheckmann

aheckmann Feb 8, 2013

Collaborator

Not in any rush whatsoever to add domain support since its still very experimental.

Collaborator

aheckmann commented Feb 8, 2013

Not in any rush whatsoever to add domain support since its still very experimental.

@aheckmann aheckmann closed this Feb 8, 2013

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Feb 8, 2013

Contributor

1337 (leet) ticket

Contributor

rauchg commented Feb 8, 2013

1337 (leet) ticket

@aheckmann

This comment has been minimized.

Show comment
Hide comment
@aheckmann

aheckmann Feb 8, 2013

Collaborator

lol

On Thu, Feb 7, 2013 at 4:32 PM, Guillermo Rauch notifications@github.comwrote:

1337 (leet) ticket


Reply to this email directly or view it on GitHubhttps://github.com/LearnBoost/mongoose/pull/1337#issuecomment-13270975.

Aaron
@aaronheckmann https://twitter.com/#!/aaronheckmann

Collaborator

aheckmann commented Feb 8, 2013

lol

On Thu, Feb 7, 2013 at 4:32 PM, Guillermo Rauch notifications@github.comwrote:

1337 (leet) ticket


Reply to this email directly or view it on GitHubhttps://github.com/LearnBoost/mongoose/pull/1337#issuecomment-13270975.

Aaron
@aaronheckmann https://twitter.com/#!/aaronheckmann

@ragulka

This comment has been minimized.

Show comment
Hide comment
@ragulka

ragulka Mar 26, 2013

Can anyone comment if the following happens (process.domain is undefined) because domains are not supported in Mongoose?

app.use(function(req,res,next)() {
  console.log(process.domain); // prints out the domain information
  UserModel.findOne({email: 'me@example.com'}, function(err, user) {
    console.log(process.domain); // undefined
    next();
  });
});

ragulka commented Mar 26, 2013

Can anyone comment if the following happens (process.domain is undefined) because domains are not supported in Mongoose?

app.use(function(req,res,next)() {
  console.log(process.domain); // prints out the domain information
  UserModel.findOne({email: 'me@example.com'}, function(err, user) {
    console.log(process.domain); // undefined
    next();
  });
});
@DrewMartin

This comment has been minimized.

Show comment
Hide comment
@DrewMartin

DrewMartin Mar 27, 2013

Yeah, that happens

Yeah, that happens

@DrewMartin DrewMartin deleted the DrewMartin:untagged branch Mar 27, 2013

@erlichmen

This comment has been minimized.

Show comment
Hide comment
@erlichmen

erlichmen Apr 27, 2013

I think that this should be merge in, domain support is pretty important and it fills the needs to go over all your code and handwrite domain.run().

The risk of supporting it is minor as the code only kicks in once a domain was set by the user.

Last point: I think that domains are definitely here to stay so we might as well gather feedback about integrating with them as soon as possible and not wait for the last minute when they will be announced and by then only have a patchy support for it.

I think that this should be merge in, domain support is pretty important and it fills the needs to go over all your code and handwrite domain.run().

The risk of supporting it is minor as the code only kicks in once a domain was set by the user.

Last point: I think that domains are definitely here to stay so we might as well gather feedback about integrating with them as soon as possible and not wait for the last minute when they will be announced and by then only have a patchy support for it.

@aheckmann

This comment has been minimized.

Show comment
Hide comment
@aheckmann

aheckmann Apr 30, 2013

Collaborator

Let me preface that I haven't spent much time with domains yet. Why is managing domains in application code not enough?

Collaborator

aheckmann commented Apr 30, 2013

Let me preface that I haven't spent much time with domains yet. Why is managing domains in application code not enough?

@erlichmen

This comment has been minimized.

Show comment
Hide comment
@erlichmen

erlichmen May 9, 2013

In a normal app, how many times are you calling mongoose? for each call you will need to wrap the callback with code that will handle the domain. This is very error prune.
I have a very small app with five models and I need to take care for the domain in around twenty places.

I had the same discussion the the package owner of bcrypt, we both agreed that node.js can do a better job of handling domains at the core level and since bcrypt is only used in very few places in the application that code can be as well taken care by the application and not by the package. This is not the case with mongoose which is called quite often by the application (as any ORM does).
That is way I think that domain handling should be done by the package and not by the application.

In a normal app, how many times are you calling mongoose? for each call you will need to wrap the callback with code that will handle the domain. This is very error prune.
I have a very small app with five models and I need to take care for the domain in around twenty places.

I had the same discussion the the package owner of bcrypt, we both agreed that node.js can do a better job of handling domains at the core level and since bcrypt is only used in very few places in the application that code can be as well taken care by the application and not by the package. This is not the case with mongoose which is called quite often by the application (as any ORM does).
That is way I think that domain handling should be done by the package and not by the application.

@sandinmyjoints

This comment has been minimized.

Show comment
Hide comment
@sandinmyjoints

sandinmyjoints Sep 9, 2013

+1 for adding domain support. There are many async io callbacks in our app. Would rather not have to bind a domain to each one.

+1 for adding domain support. There are many async io callbacks in our app. Would rather not have to bind a domain to each one.

@jmendiara

This comment has been minimized.

Show comment
Hide comment

+1

@labajo

This comment has been minimized.

Show comment
Hide comment

labajo commented Oct 3, 2013

+1

@palmerabollo

This comment has been minimized.

Show comment
Hide comment

+1

@gyllstromk

This comment has been minimized.

Show comment
Hide comment
@gyllstromk

gyllstromk Nov 7, 2013

I think the support needs to be in the node-mongodb-native module and not in mongoose.

I believe the way the driver library receives responses from the binary layer breaks the domain continuity (i.e. responses trigger the callbacks from outside the domain).

The only way mongoose could solve the problem is by wrapping each callback sent to the mongo driver with domain.active.bind(). i.e. the wrapping would need to be in many more places than it is in the current PR. I think that is not an ideal approach for many reasons.

Probably best to handle on the client side or file a request to the mongo-driver team.

I think the support needs to be in the node-mongodb-native module and not in mongoose.

I believe the way the driver library receives responses from the binary layer breaks the domain continuity (i.e. responses trigger the callbacks from outside the domain).

The only way mongoose could solve the problem is by wrapping each callback sent to the mongo driver with domain.active.bind(). i.e. the wrapping would need to be in many more places than it is in the current PR. I think that is not an ideal approach for many reasons.

Probably best to handle on the client side or file a request to the mongo-driver team.

@cyberwombat

This comment has been minimized.

Show comment
Hide comment

+1

@sahat

This comment has been minimized.

Show comment
Hide comment
@sahat

sahat Dec 20, 2013

+1 for domain support.

sahat commented Dec 20, 2013

+1 for domain support.

@ashtuchkin

This comment has been minimized.

Show comment
Hide comment
@ashtuchkin

ashtuchkin Dec 24, 2013

I filed a feature request in the driver:
https://jira.mongodb.org/browse/NODE-110

I filed a feature request in the driver:
https://jira.mongodb.org/browse/NODE-110

@christkv

This comment has been minimized.

Show comment
Hide comment
@christkv

christkv Dec 24, 2013

still marked as unstable so will not support for now

still marked as unstable so will not support for now

@gyllstromk

This comment has been minimized.

Show comment
Hide comment
@gyllstromk

gyllstromk Dec 24, 2013

I agree with the mongo team to defer until node domains mature. I advise closing this issue as I don't think mongoose is the correct layer for a solution, and because a solution is probably relatively far in the future, and further +1s to this issue are probably likely.

I agree with the mongo team to defer until node domains mature. I advise closing this issue as I don't think mongoose is the correct layer for a solution, and because a solution is probably relatively far in the future, and further +1s to this issue are probably likely.

@sahat

This comment has been minimized.

Show comment
Hide comment
@sahat

sahat Dec 24, 2013

Understandable. So what should we use as a workaround if we want to wrap mongoose.connect with a domain, but at the same time avoid crashing the server on unhandled save() error, or findOne() error?

sahat commented Dec 24, 2013

Understandable. So what should we use as a workaround if we want to wrap mongoose.connect with a domain, but at the same time avoid crashing the server on unhandled save() error, or findOne() error?

@cyberwombat

This comment has been minimized.

Show comment
Hide comment
@cyberwombat

cyberwombat Dec 24, 2013

Just my 2 cents here.. I recently tried to setup domains on an Express app to catch errors - while I could have left Mongoose calls alone in most cases it turned out it was not possible to use Mongoose in middleware without wrappingeach and every query with domain support. As I experienced it domains are not useable until Mongoose supports them. I had to abandon domains due to the sheer amount of work involved.

Just my 2 cents here.. I recently tried to setup domains on an Express app to catch errors - while I could have left Mongoose calls alone in most cases it turned out it was not possible to use Mongoose in middleware without wrappingeach and every query with domain support. As I experienced it domains are not useable until Mongoose supports them. I had to abandon domains due to the sheer amount of work involved.

@sahat

This comment has been minimized.

Show comment
Hide comment
@sahat

sahat Dec 24, 2013

It's a little frustrating how something so common cannot be handled properly. Why does mongoose crash the server even if I try to catch an error in a callback?

mongoose.connection('localhost', function(err) {
  if (err) console.log('Do nothing.');
});

A second later...

Error: failed to connect to [localhost:27017]
    at null.<anonymous> (/<hidden>/node_modules/mongoose/node_modules/mongodb/lib/mongodb/connection/server.js:540:74)
    at EventEmitter.emit (events.js:106:17)
    at null.<anonymous> (/<hidden>/node_modules/mongoose/node_modules/mongodb/lib/mongodb/connection/connection_pool.js:140:15)
    at EventEmitter.emit (events.js:98:17)
    at Socket.<anonymous> (/<hidden>/node_modules/mongoose/node_modules/mongodb/lib/mongodb/connection/connection.js:478:10)
    at Socket.EventEmitter.emit (events.js:95:17)
    at net.js:441:14
    at process._tickCallback (node.js:415:13)

Process finished with exit code 8

And this happens even if I pass options object with auto_reconnect: true. I even attempted this Express middleware approach to catch mongoose errors and try to reconnect, only to see the same Process finished with exit code 8:

app.use(function(req, res, next) {
  if (1 !== mongoose.connection.readyState) {
    mongoose.connect(config.db, options);
    res.send(500, 'MongoDB unavailable');
  }
  next();
});

Domains is the only solution that worked for me, but it also creates other problems since mongoose or mongodb driver does not support domains yet.

sahat commented Dec 24, 2013

It's a little frustrating how something so common cannot be handled properly. Why does mongoose crash the server even if I try to catch an error in a callback?

mongoose.connection('localhost', function(err) {
  if (err) console.log('Do nothing.');
});

A second later...

Error: failed to connect to [localhost:27017]
    at null.<anonymous> (/<hidden>/node_modules/mongoose/node_modules/mongodb/lib/mongodb/connection/server.js:540:74)
    at EventEmitter.emit (events.js:106:17)
    at null.<anonymous> (/<hidden>/node_modules/mongoose/node_modules/mongodb/lib/mongodb/connection/connection_pool.js:140:15)
    at EventEmitter.emit (events.js:98:17)
    at Socket.<anonymous> (/<hidden>/node_modules/mongoose/node_modules/mongodb/lib/mongodb/connection/connection.js:478:10)
    at Socket.EventEmitter.emit (events.js:95:17)
    at net.js:441:14
    at process._tickCallback (node.js:415:13)

Process finished with exit code 8

And this happens even if I pass options object with auto_reconnect: true. I even attempted this Express middleware approach to catch mongoose errors and try to reconnect, only to see the same Process finished with exit code 8:

app.use(function(req, res, next) {
  if (1 !== mongoose.connection.readyState) {
    mongoose.connect(config.db, options);
    res.send(500, 'MongoDB unavailable');
  }
  next();
});

Domains is the only solution that worked for me, but it also creates other problems since mongoose or mongodb driver does not support domains yet.

@christkv

This comment has been minimized.

Show comment
Hide comment
@christkv

christkv Dec 24, 2013

the 1.3.23 driver should work ok with domains but the current mongoose is still using 1.3.19 which does not.

the 1.3.23 driver should work ok with domains but the current mongoose is still using 1.3.19 which does not.

@christkv

This comment has been minimized.

Show comment
Hide comment
@christkv

christkv Dec 24, 2013

using domains however extensively in your app will set you up for some painful experiences going forward as it's my understanding that the current implementation is considered to not be good and I expect it to change a fair bit before node 1.0 where I would hope it would go stable. Streams are going through a bit of the same aswell.

using domains however extensively in your app will set you up for some painful experiences going forward as it's my understanding that the current implementation is considered to not be good and I expect it to change a fair bit before node 1.0 where I would hope it would go stable. Streams are going through a bit of the same aswell.

@aheckmann

This comment has been minimized.

Show comment
Hide comment
@aheckmann

aheckmann Jan 10, 2014

Collaborator

mongoose 3.8.4 has the updated driver now.

On Tuesday, December 24, 2013, Christian Amor Kvalheim wrote:

using domains however extensively in your app will set you up for some
painful experiences going forward as it's my understanding that the current
implementation is considered to not be good and I expect it to change a
fair bit before node 1.0 where I would hope it would go stable. Streams are
going through a bit of the same aswell.


Reply to this email directly or view it on GitHubhttps://github.com/LearnBoost/mongoose/pull/1337#issuecomment-31182846
.

Aaron
@aaronheckmann https://twitter.com/#!/aaronheckmann
soundcloud.com/ajhecky
github.com/aheckmann

Collaborator

aheckmann commented Jan 10, 2014

mongoose 3.8.4 has the updated driver now.

On Tuesday, December 24, 2013, Christian Amor Kvalheim wrote:

using domains however extensively in your app will set you up for some
painful experiences going forward as it's my understanding that the current
implementation is considered to not be good and I expect it to change a
fair bit before node 1.0 where I would hope it would go stable. Streams are
going through a bit of the same aswell.


Reply to this email directly or view it on GitHubhttps://github.com/LearnBoost/mongoose/pull/1337#issuecomment-31182846
.

Aaron
@aaronheckmann https://twitter.com/#!/aaronheckmann
soundcloud.com/ajhecky
github.com/aheckmann

@coolgeng

This comment has been minimized.

Show comment
Hide comment

@aheckmann thanks.

@vkarpov15

This comment has been minimized.

Show comment
Hide comment
@vkarpov15

vkarpov15 Mar 15, 2015

Collaborator

FWIW domains are really buggy, I would recommend avoiding them. More modern alternatives include zones.

Collaborator

vkarpov15 commented Mar 15, 2015

FWIW domains are really buggy, I would recommend avoiding them. More modern alternatives include zones.

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