Even with synchronized block getting a duplicate value

Question

my class has a method as follow :

private volatile String currentSeqSecondBucket = "";
private volatile int currentUriSeq = 0;

public String calculateRepositoryURI(Date now, String userSpecifiedFolder)
{
  String folder = userSpecifiedFolder;

  if (StringUtils.isBlank(folder))
    folder = RSIConstant.CONTENT_ROOT_FOLDER;

  StringBuilder buf = new StringBuilder(folder);

  if (!folder.endsWith("/"))
    buf.append("/");

  String nowStr = YYYYMMDDHHMMSS_FORMAT.format(now);

  buf.append(String.valueOf((int)(Math.random() * 1000)));
  buf.append("/");
  buf.append(nowStr);
  buf.append("_"); 


  synchronized (this)// here the this means my class
  {
    if (currentSeqSecondBucket.equals(nowStr))
    {
      currentUriSeq++;
    }
    else
    {
      currentUriSeq = 1;
      currentSeqSecondBucket = nowStr;
    }
    buf.append(String.valueOf(currentUriSeq));
  }

  buf.append(".xml");

  return buf.toString();
}

Will this code return a unique string for every call?

There could be many things which could have caused the duplication. Like `currentUriSeq` being accessible from other threads. Request you to provide the whole code of your's to analyze — Ashishkumar Singh, Oct 29 '18 at 08:17
@sidgate If you edit a post, please fix all the problems with it — Michael, Oct 29 '18 at 08:18
why are you synchronizing on `this` - `this` is a different object for each thread. — Scary Wombat, Oct 29 '18 at 08:18
It doesn't return anything :P If the question is if currentUriSeq can become larger than 1, yes, if two subsequent nowStr are equal. — user1587520, Oct 29 '18 at 08:19
Quite obviously this code will not always return a unique value, because there is a conditional statement which reset it back to 1. — Michael, Oct 29 '18 at 08:20
@ScaryWombat huh? It's perfectly reasonable to synchronize on `this`. If what you were saying were true, there would be no reason to ever synchronize on it. — Michael, Oct 29 '18 at 08:22
@Michael Sorry, based on the lack of code, you are correct. I was maybe mis-reading that the OP wanted to prevent mutliple Threads calling `one` method overwriting the value of a variable. Still https://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java is an interesting read — Scary Wombat, Oct 29 '18 at 08:27
@ScaryWombat Indeed, that's why Lombok has [`@Synchronized`](https://projectlombok.org/features/Synchronized) — Michael, Oct 29 '18 at 08:32
well, we have no idea what `this` and all the *variables* or *fields* are. Please consider posting a [mcve]. Also your question is pretty open, and here the answer: "no, it will not return unique `currentUriSeq` - it is conditionally being set to `1` (even for same `nowStr` if there was a call with a different `nowStr` in between) — user85421, Oct 29 '18 at 08:40

Stephen C · Answer 1 · 2018-10-29T22:34:07.407

Will this code return a unique string for every call?

No. It won't. But the reason is nothing to do with thread safety.

The reason is that occasionally you may get the same random number generated twice in the same second. Now if the same random number was generated twice in a row, then your test currentSeqSecondBucket.equals(nextStr) would catch this and you would increment currentUriSeq and avoid the collision. However, a more likely scenario is that the random number repeats after more than one call in the same second. That will defeat your test.

My advice would be to not try to re-invent the wheel:

If you want non-guessable unique names, use the UUID class to give you uniqueness.
If you don't care about guessability, just use a simple sequence number; e.g. implemented using AtomicInteger or AtomicLong.

Even with synchronized block getting a duplicate value

1 Answers1