aspose file tools*
The moose likes Threads and Synchronization and the fly likes Realability on Id Generation Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Java » Threads and Synchronization
Bookmark "Realability on Id Generation" Watch "Realability on Id Generation" New topic
Author

Realability on Id Generation

vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

Hi
I have below code which i use to generate Ids for my file.


There can be hundreds of user create the file same time and access that file to generate Id for that file. Can i rely on above code to get the ID. Just want to apply synchronization on the generateIdforFile() so that only one user get the unique ID.

Thanks
James Boswell
Bartender

Joined: Nov 09, 2011
Posts: 1030
    
    5

Vivek

One thing I will point out is that you do not need the statementin a synchronized method. It is implicit.
Steve Luke
Bartender

Joined: Jan 28, 2003
Posts: 4181
    
  21

This may or may not be safe (remember to follow James Boswell's advice) depending on how clients get to the generateIdForFile() method. If they do new GenerateId().generateIdForFile() then no, this is not safe. All clients must be using the same instance of GenerateId.

It also isn't very efficient. Why do you need to open the file, read through it, close it, open it again, write to it, and close it again for each call? What is the purpose for the file? Could it be replaced with an AtomicInteger (i.e. no persistence) or volatile int in memory with a properties file for persistence (only care about the last value)? Or volatile int in memory with a RandomAccessFile that would let you skip to the end and store the last value (need to store all ids)?

If you do need to both read and write from the file I would consider the RandomAccessFile anyway, and keep it open. That way you can read and write to/from the file without constantly opening and closing the file. I would also think about caching the value in memory so you don't need to read it from the file every time.


Steve
vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

Hi

Clients will access this using new GenerateId().generateIdForFile(). I will use the RandomAccess to find the end value. But as you said it will not be thread safe, so should i make the GenerateId class singleton or if i make the function static will solve it or any other way. We are using Java6.

Thanks
Martin Vajsar
Sheriff

Joined: Aug 22, 2010
Posts: 3611
    
  60

Don't you use a database in your project? If you do, using the database to safely generate the unique keys might be the easiest way.
vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

Hi

Database is there, but some champs started it wrong. I have done some modifications in my code to prevent the duplicate id generation if multiple users will create the files at one go.


And Client will access it using
Some modification yet not done as recommended by Steve. Now trying to fix the issue that i have.
Salil Vverma
Ranch Hand

Joined: Sep 06, 2009
Posts: 255

vivek dhiman wrote:Hi

Database is there, but some champs started it wrong. I have done some modifications in my code to prevent the duplicate id generation if multiple users will create the files at one go.


And Client will access it using
Some modification yet not done as recommended by Steve. Now trying to fix the issue that i have.


Hey Vivek,

your code still looks to be pone to concurrency issues. You have removed multiple synchronization from your code (as suggested by Steve) , which is good. you have made your method static synchronized to make sure that always class level lock is taken and only one thread will be accessing it at a time.

But the variables incrementedVal and initialVal are not volatile. This will increase the chances of second thread start working on stale value of these two variables, which might again cause unintended behavior.

Regards
Salil Verma
vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

Thanks, So If i make


will be OK?
Salil Vverma
Ranch Hand

Joined: Sep 06, 2009
Posts: 255

Hey Vivek,

From concurrency perspective. The code would be all right after adding volatile in the static fields mentioned your previous post.
vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

Thanks, Can anyone tell me if i use below code, will it be also threadsafe? And will call this function with class name

Steve Luke
Bartender

Joined: Jan 28, 2003
Posts: 4181
    
  21

Salil Vverma wrote:Hey Vivek,

From concurrency perspective. The code would be all right after adding volatile in the static fields mentioned your previous post.

Volatile is not needed, since the values are only used in the synchronized block/method. Volatile is needed to make sure multiple threads see the latest state of the variable. The synchronized method provides memory barriers that ensure the values are published to threads before they are read (as long as they are also read in a synchronized block or method). Volatile is redundant, probably a little inefficient, and a bit misleading.

I would actually question why those variables need to be in the class scope. They always get assigned new values in the method, and never rely on the previous value to work correctly. I would move both variables into the method scope.

You open a new FileReader every time the method is called and never close it, then you use the suppress warnings annotation to ignore the very meaningful warning that you are opening a resource and never closing it. This leads to a memory leak. You should close those resources. I told you before about trying to avoid opening and closing files, but the point was that you need to keep using the same one, not making new ones and leaving them open.
Steve Luke
Bartender

Joined: Jan 28, 2003
Posts: 4181
    
  21

vivek dhiman wrote:Thanks, Can anyone tell me if i use below code, will it be also threadsafe? And will call this function with class name


Yes, that should be safe, and much better than your file alternative.
Salil Vverma
Ranch Hand

Joined: Sep 06, 2009
Posts: 255

Volatile is not needed, since the values are only used in the synchronized block/method. Volatile is needed to make sure multiple threads see the latest state of the variable. The synchronized method provides memory barriers that ensure the values are published to threads before they are read (as long as they are also read in a synchronized block or method). Volatile is redundant, probably a little inefficient, and a bit misleading.


Hey Steve,

Ah.. you are right.. Synchronized method's memory barrier would ensure the value publishing so volatile is not needed. I had missed the memory barrier concept while suggesting the solution.
Thanks for pointing it out.
vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

Thanks a ton . Last point : Will it produce the unique ids after restarting the Application. ? like it should not repeat the values that already generated before application stop and after application restart.
Martin Vajsar
Sheriff

Joined: Aug 22, 2010
Posts: 3611
    
  60

The probability that two random UUIDs will be identical is vanishingly small, so small that it can probably be ignored for most practical purposes.

However, UUIDs are somewhat impractical in my opinion. Its String representation is pretty long. If you need to generate the IDs for database keys, definitely use database mechanisms for that. If you need the UUIDs for something which is not inside the database, but unique to your application, then I'd suggest to use the database mechanisms too. Only if you need the IDs to be truly random (eg., not guessable by someone), UUIDs might be more practical than the DB implementation.
vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

DB is the last option as per current situation in our project.
I tried to use new approach here to use to generate Ids for my Files.


As UUID create lonng ID,s so trying to find other approach.
Among UUID and AtomicInteger which one is better without compromising ID Uniqueness which should not break in either approach. Also I have to keep this under consideration that ID value should not be duplicate after restart the application and this should be threadsafe.
Martin Vajsar
Sheriff

Joined: Aug 22, 2010
Posts: 3611
    
  60

AtomicInteger (or AtomicLong) will generate unique IDs in one run of the application, assuming no overflow. It then depends on how you initialize it. Your latest code could create duplicates after restarting the application in at least two different ways:

1) Somebody adjusts the time on the computer backwards.

2) You generate on average more than a million IDs per second (there are 1000 milliseconds per second, and you're multiplying the System.currentTimeMillis() by another thousand).

Oh, and of course, if you run two instances of the application in parallel, you can get duplicates as well. If there are other ways to create duplicates, I haven't spotted them.
Ulf Dittmer
Marshal

Joined: Mar 22, 2005
Posts: 42648
    
  65
Or you could use a single static AtomicInteger that's initialized with 0, and use the return value of its incrementAndGet method as the ID. That's an atomic operation. If you prepend that with a timestamp of the startup time of your JVM it'll be a unique value. Something like this:


In this case, I think the method wouldn't even need to be synchronized.


Ping & DNS - my free Android networking tools app
Ulf Dittmer
Marshal

Joined: Mar 22, 2005
Posts: 42648
    
  65
Martin Vajsar wrote:if you run two instances of the application in parallel, you can get duplicates as well.

Good point! Appending the IP address would cover that case.
vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

Hi I have update the code


Can this is prone to duplicate in any case, apart from clock time go backward mannually.
Martin Vajsar
Sheriff

Joined: Aug 22, 2010
Posts: 3611
    
  60

I'd say it could be:

1) The system time doesn't change every millisecond. On Windows, the resolution is usually around few tens of milliseconds, but it may wary. In any case, if you don't know the resolution for certain, you need to assume the worst. If you happen to generate more than 1000 IDs before System.currentTimeMillis() value changes, you'll get duplicates.

2) I don't know what's the current value of Long.toString(System.currentTimeMillis(), 36) is, but sooner or later, this number will get an additional digit. At that time, it is possible that an old ID glued together with 2 digit counter will equal to a recent ID glued together with a 1 digit counter. You could make sure this won't happen by left-padding the two numbers with zeroes to a standard with.

I my opinion your previous try was better.

I'd like to reiterate the point about concurrent runs of your application - if you happen to run the application twice even inadvertently, then, assuming both copies would be fully functional, it might start to generate duplicates. App servers usually prevent the applications from running in multiple copies, but if you plan to schedule your application using other means, you need to take extra precautions against running it twice on the same machine (in which case even Ulf's suggestion to append the IP addresses wouldn't work).

If you can easily obtain the last ID generated (eg. by a database query), you might use it to initialize your counter to start from there. You'd have to make really sure that no new IDs can be generated while you initialize the counter.
vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

OK i'll try to get the DB approach and then will post the updated scenario.
Ulf Dittmer
Marshal

Joined: Mar 22, 2005
Posts: 42648
    
  65
I still think that the approach I outlined is simpler to implement. It also involves no network activity. Is there something about it that you think might be problematic?
vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

So As per you Ulf below code will not generate duplicate Ids, In either case Application/server restarts and also after that Ids will unique. And is completely threadsafe. Only Exception i count someone not manually change the system clock.

To make it more narrow, can we also use System.nanoTime() instead System.currentTimeMillis().
Ulf Dittmer
Marshal

Joined: Mar 22, 2005
Posts: 42648
    
  65
Right, if you set the system clock back that could happen. But it would be very unlikely that the startup value would be exactly the same.
vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

Below Code i'll use finally.



Thanks All.
Ulf Dittmer
Marshal

Joined: Mar 22, 2005
Posts: 42648
    
  65
...it always giving me 1381768314645-1 Here -1 coming in all values.

That doesn't happen for me. Please post the SSCCE that you're using to test this. To wit:

produces:


As an aside, what is to toString() call in line 9 of your code supposed to accomplish? It's redundant.

And What about that, will it be more efficient ...

I see nothing in the javadocs of the RuntimeMXBean.getName() method that would lead me to believe that its return value is unique in any way (where did you get the idea that it is?), so the question of efficiency doesn't even arise.
Ulf Dittmer
Marshal

Joined: Mar 22, 2005
Posts: 42648
    
  65
It's great that you got this to work, but no thanks for editing your post so that my reply -in which I put some work- now looks silly.

Line 6 of your code is still redundant.
vivek dhiman
Ranch Hand

Joined: Aug 05, 2011
Posts: 130

Yes I changed the post meanwhile i was testing this. Quick Response, that's why this is my favorite forum. So I changes to System.nanoTime() which i don't think will put much effect.
Tested this code to generate 1000000 unique ids using threads.
Ulf Dittmer
Marshal

Joined: Mar 22, 2005
Posts: 42648
    
  65
I think you misunderstood both parts of my last post, but let's not dwell on that. Marking this topic as solved.
 
With a little knowledge, a cast iron skillet is non-stick and lasts a lifetime.
 
subject: Realability on Id Generation