Avoid sharing sockets between forked processes

[eugeneius]

Support for detecting when a socket was established in a different process and automatically discarding it was added to this driver in 997d9b8, and refined in a393557.

Instances of the Node class maintain their own sockets for refreshing replica set data; they need to be protected from reuse in the same way.

[estolfo]

Hi @eugeneius

We can't use Monitor because we support ruby 1.8.7 in the 1.x series of the driver.

Is there a particular test case you can share that recreates the problem you are trying to solve? That way, we can see how to resolve it without Monitor.

Also, we have released an rc of a rewrite of the ruby driver. It's available on RubyGems as 2.0.0.rc.
https://rubygems.org/gems/mongo/versions/2.0.0.rc
You might want to try it out to see if the issues you are having are resolved.

Thanks
Emily

[eugeneius]

The test suite passes on 1.8.7 - is there a known issue with Monitor which means it can't be used? I replaced the Mutex with a Monitor because with this patch connect can be called inside set_config and both need to hold the lock; Monitor is reentrant, while Mutex isn't. It should be possible to make this patch work with a Mutex, but it might involve rearranging the class a bit.

I opened a ticket describing the issue I'm seeing: https://jira.mongodb.org/browse/RUBY-877

It only happens a few dozen times a day in production, and I haven't been able to reproduce it locally.

Even if 2.0.0.rc fixes the problem I won't be able to use it in my application, because I'm using MongoMapper which relies on 1.x.

[durran]

If the process is not forked, @socket.pid will always return nil, which will cause the socket to be closed and reopened on every request, thus causing a significant negative impact on performace.

[eugeneius]

The pid property is assigned when a socket is created:

https://github.com/mongodb/mongo-ruby-driver/blob/1.12.0/lib/mongo/connection/socket/ssl_socket.rb#L28
https://github.com/mongodb/mongo-ruby-driver/blob/1.12.0/lib/mongo/connection/socket/tcp_socket.rb#L27
https://github.com/mongodb/mongo-ruby-driver/blob/1.12.0/lib/mongo/connection/socket/unix_socket.rb#L28

This is the same logic being used already in the connection pool code:

https://github.com/mongodb/mongo-ruby-driver/blob/1.12.0/lib/mongo/connection/pool.rb#L224

[durran]

Sorry, missed that part, was thinking of IO#pid in core Ruby. :)

[estolfo]

My mistake, I did a quick test with 1.8.7 and you have to require 'monitor' in order to use it, whereas you don't with >= 1.9.3

I'm going to do a bit more testing but this patch seems reasonable. Thanks for opening the ticket and taking the time to investigate.

[estolfo]

Ideally, we would have a test for this scenario. If you can write one, that would be great. Otherwise, I'll add one.

[estolfo]

Hi Eugene

Thanks again for your contribution.

I've taken a closer look at your PR and have a few changes to propose. They can be found in this branch and are being tested in jenkins here.

My suggestions are:

1. Changing the name of the socket method to #usable_socket. This makes it more explicit that this method needs to be called when the node's @socket is used.
2. Making the #usable_socket method private because with your change, called node.socket could mutate the node instance; the method gets the node lock and potentially calls #connect on the node. I think we should keep the accessor method #socket immutable on the node class.
3. Adding a test that gets the socket associated with a node, changes its pid, then triggers an ismaster command. The expected outcome is that the node gets a new socket because the old socket's pid does not match the process's pid.

Let me know if you'd like to update the PR yourself so I can accept it or if you want me to merge it, then make these changes on top.

Thanks again

Emily

[eugeneius]

Thanks for the code review @estolfo! I've cherry-picked your changes into this branch.

[estolfo]

Perfect, thanks @eugeneius !

[estolfo]

oh, @eugeneius actually, can you rebase because I commit today? 561e1a6
Thanks

[eugeneius]

Done!

[estolfo]

hey @eugeneius !
ruby driver version 1.12.1 has been released with your PR. Feel free to try it out!
Thanks again

[eugeneius]

Thanks @estolfo, we're upgrading now :)