Skip to content

Conversation

mcruzdev
Copy link

@mcruzdev mcruzdev commented Sep 25, 2025

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:

  • Remove the com.github.f4b6a3:ulid-creator dependency.
  • Add a new dependency de.huxhorn.sulky:de.huxhorn.sulky.ulid for ULID generation.
  • Add a default WorkflowInstanceIdFactory implementation (UlidWorkflowInstanceIdFactory)
  • Sort dependencies version

Closes #812

@mcruzdev mcruzdev requested a review from fjtirado as a code owner September 25, 2025 18:52
Comment on lines 27 to 39
private final SecureRandom random = new SecureRandom();
private final ULID ulid = new ULID(random);
private ULID.Value previousUlid;

@Override
public synchronized String get() {
if (previousUlid == null) {
previousUlid = ulid.nextValue();
} else {
previousUlid = ulid.nextMonotonicValue(previousUlid);
}
return previousUlid.toString();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be more lazy.
Reason is that, with this implementaiton, if the user is replacing this factory, the secure ramdon and the ULID are still created, but never used.
There are two possibilities to achieve that:

  • creare this class, as it is, (well, secureRamdon does not need to be a member in any case) in a lazy way in the WorkflowApplication (so create it only when the user has not set another WorkflowInsanceIdFactory)
  • Leave WorkflowApplication as it is and create the ULID (and the SecureRamdon) when previousUlid is null.

Copy link
Collaborator

@fjtirado fjtirado Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now explain why I prefer the first one (lazy initialization of the factory in WorkflowApplication)

The get method can be simplified by creating previousUlid in the constructor, so you write a not synchronized get.

@Override
// note that you do not need to syncronized this
  public String get() {
       String result=  currentUlid.toString();
      currentUlid = ulid.nextMonotonicValue(currentUlid);
      return result;
  }

The constructor will look like

private final ULID ulid;
private ULID.Value currentUlid;

public MonotonicUlidWorkflowInstanceIdFactory () {
   ulid = new ULID(new SecureRandom());
   currentUlid = ulid.nextValue();
}

And in workflowapplicationbuilder.build you set the factory to MonoloictUlidWorkflowInstanceFactory if the id factory it is null (the user has not set its own one) rather than in builder initialization

Copy link
Collaborator

@fjtirado fjtirado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcruzdev Posible solution proposed, please take a look

Comment on lines 27 to 39
private final SecureRandom random = new SecureRandom();
private final ULID ulid = new ULID(random);
private ULID.Value previousUlid;

@Override
public synchronized String get() {
if (previousUlid == null) {
previousUlid = ulid.nextValue();
} else {
previousUlid = ulid.nextMonotonicValue(previousUlid);
}
return previousUlid.toString();
}
Copy link
Collaborator

@fjtirado fjtirado Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now explain why I prefer the first one (lazy initialization of the factory in WorkflowApplication)

The get method can be simplified by creating previousUlid in the constructor, so you write a not synchronized get.

@Override
// note that you do not need to syncronized this
  public String get() {
       String result=  currentUlid.toString();
      currentUlid = ulid.nextMonotonicValue(currentUlid);
      return result;
  }

The constructor will look like

private final ULID ulid;
private ULID.Value currentUlid;

public MonotonicUlidWorkflowInstanceIdFactory () {
   ulid = new ULID(new SecureRandom());
   currentUlid = ulid.nextValue();
}

And in workflowapplicationbuilder.build you set the factory to MonoloictUlidWorkflowInstanceFactory if the id factory it is null (the user has not set its own one) rather than in builder initialization

@mcruzdev
Copy link
Author

Reason is that, with this implementaiton, if the user is replacing this factory, the secure ramdon and the ULID are still created, but never used.

Good catch!

The get method can be simplified by creating previousUlid in the constructor, so you write a not synchronized get.

@fjtirado, do we need to guarantee no duplicated keys in a thread-safe way? If not, I agree with you to remove the synchronized or similar locks.

@fjtirado
Copy link
Collaborator

fjtirado commented Sep 26, 2025

@mcruzdev
To avoid synchronization (and the risk derived from not using it), we can write

private final AtomicReference<ULID.Value> currentUlid;

public MonotonicUlidWorkflowInstanceIdFactory () {
   ulid = new ULID(new SecureRandom());
   currentUlid = new AtomicReference<>(ulid.nextValue());
}
 public String get() {   
      return currentUlid.getAndSet(ulid.nextMonotonicValue(currentUlid));
  }

Note: Im asuming nextMonotoicValue is thread-safe (it wont crash when invoked simoultaneuously from different threads),

@mcruzdev
Copy link
Author

I added the updateAndGet to avoid race window and the user gets a ulid closer to the startedAt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native-image build fails due to UlidCreator static initialization (Random in image heap)
3 participants