Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Synchronize access to Server#next_uuid #88

Open
wants to merge 1 commit into from

2 participants

@ferrous26

In threaded environments, multiple threads might be requesting a UUID at the same time.

This causes problems on JRuby where Array objects are not thread safe and calling
#pop was returning the same value to multiple threads!

@samlown
Owner

Thanks for that, seems to make sense to me. However, I've never really been convinced about the next_uuid stuff when saving documents, I can't see the point when the database can do this for you without the use of mutexes. The save_doc [1] method is especially crappy with far too many rescues for older versions.

The reasoning for using UUIDs seems to be out of fear for proxies in the middle resending POST requests [2], I honestly cannot believe that is a problem, especially not for server to server calls.

Do you have a special use case for next_uuid or are you just trying to save documents?

Cheers,
sam

[1] https://github.com/couchrest/couchrest/blob/master/lib/couchrest/database.rb#L114
[2] http://wiki.apache.org/couchdb/HTTP_Document_API#line867

@ferrous26

I do not have a special use case for #next_uuid, I'm just saving new documents through couchrest_model. I was hoping to keep using UUID's as the unique identifier for documents in a particular database.

It would be great if I could just rely on CouchDB itself to assign the UUID on the server side. couchrest seems to use #put in the #save_doc method; I don't see why a proxy in the middle would resend that as a #post, but I don't have a super proficient understanding of HTTP.

Maybe we could simplify #save_doc to save the document right away, even if it has no _id set, and just leave #next_uuid for backwards compatibility?

@ferrous26

Whoops, I didn't read the documentation correctly...we do want to use #post when there is no _id...hmmm

@ferrous26

I think the best solution to this issue is simply to find an alternative way to generate UUIDs on the client. So I've done that instead.

Thank you for the pointers.

@ferrous26

The solution to this problem, I think, is to use the standard Ruby library securerandom to generate all UUID's.

I changed my pull request accordingly, but now a test that depended on some mock data is failing. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 15 additions and 5 deletions.
  1. +4 −5 lib/couchrest/server.rb
  2. +11 −0 spec/couchrest/server_spec.rb
View
9 lib/couchrest/server.rb
@@ -1,3 +1,5 @@
+require 'securerandom'
+
module CouchRest
class Server
attr_accessor :uri, :uuid_batch_count, :available_databases
@@ -80,12 +82,9 @@ def restart!
end
# Retrive an unused UUID from CouchDB. Server instances manage caching a list of unused UUIDs.
+ # The count argument is deprecated and has no effect anymore.
def next_uuid(count = @uuid_batch_count)
- @uuids ||= []
- if @uuids.empty?
- @uuids = CouchRest.get("#{@uri}/_uuids?count=#{count}")["uuids"]
- end
- @uuids.pop
+ SecureRandom.uuid
end
end
View
11 spec/couchrest/server_spec.rb
@@ -30,6 +30,17 @@
@couch.default_database = 'cr-server-test-default-db'
@couch.available_database?(:default).should be_true
end
+
+ it "should synchronize access to #next_uuid" do
+ uuids1 = []
+ uuids2 = []
+ Thread.new @couch do |server|
+ 500.times { uuids1 << server.next_uuid }
+ end
+ 500.times { uuids2 << @couch.next_uuid }
+ sleep 0.001 until uuids1.size == 500
+ (uuids1 + uuids2).uniq.size.should be_equal (uuids1 + uuids2).size
+ end
end
end
Something went wrong with that request. Please try again.