[
https://issues.apache.org/jira/browse/OFBIZ-2353?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14115372#comment-14115372
]
Jacopo Cappellato commented on OFBIZ-2353:
------------------------------------------
[~jacques.le.roux] you write a lot! but I am sorry to say that your previous
comments confirm to me that you do not know well enough the way the
SequenceUtil class is supposed to work.
I will try to find some time to reply to all your comments but in the meantime
I will answer your question (you asked if I found issues with clustered set to
true): the answer is yes and the proof is in the following unit test:
{code}
public void testSequenceValueItemWithConcurrentThreads() {
final SequenceUtil sequencer = new
SequenceUtil((GenericDelegator)delegator,
delegator.getGroupHelperInfo(delegator.getEntityGroupName("SequenceValueItem")),
delegator.getModelEntity("SequenceValueItem"),
"seqName", "seqId");
UUID id = UUID.randomUUID();
final String sequenceName = "BogusSequence" + id.toString();
final ConcurrentMap<Long, Long> seqIds = new ConcurrentHashMap<Long,
Long>();
final AtomicBoolean duplicateFound = new AtomicBoolean(false);
final AtomicBoolean nullSeqIdReturned = new AtomicBoolean(false);
List<Future<Void>> futures = new ArrayList<Future<Void>>();
Callable getSeqIdTask = new Callable() {
public Callable<Void> call() throws Exception {
Long seqId = sequencer.getNextSeqId(sequenceName, 1,
null);
if (seqId == null) {
nullSeqIdReturned.set(true);
return null;
}
Long existingValue = seqIds.putIfAbsent(seqId, seqId);
if (existingValue != null) {
duplicateFound.set(true);
}
return null;
}
};
Callable refreshTask = new Callable() {
public Callable<Void> call() throws Exception {
sequencer.forceBankRefresh(sequenceName, 1);
return null;
}
};
double probabilityOfRefresh = 0.1;
for (int i = 1; i <= 10000; i++) {
Callable randomTask = Math.random() < probabilityOfRefresh ?
refreshTask : getSeqIdTask;
futures.add(ExecutionPool.GLOBAL_FORK_JOIN.submit(randomTask));
}
ExecutionPool.getAllFutures(futures);
assertFalse("Null sequence id returned", nullSeqIdReturned.get());
assertFalse("Duplicate sequence id returned", duplicateFound.get());
}
{code}
As I mentioned earlier, the failure is due to bad implementation of thread safe
patterns, not to the collision algorithm that you have modified.
The approach you have followed is not wrong in itself ("acquiring a lock before
reading" is indeed the right approach and I am going to provide a clean room
implementation of it soon to replace all this mess) but it is wrong because you
have added it to an algorithm that was using a collision detection approach:
your change altered completely the flow in the code making it very difficult to
read. You have "fixed" the collisions, but the collisions were not errors but
integral part of the algorithm.
As a side note, the fix for the unit test case is a one line change that I am
going to commit tomorrow (together with the unit tests).
> SequenceUtil may generate duplicate IDs in Load Balancing mode
> ---------------------------------------------------------------
>
> Key: OFBIZ-2353
> URL: https://issues.apache.org/jira/browse/OFBIZ-2353
> Project: OFBiz
> Issue Type: Bug
> Components: framework
> Affects Versions: Release Branch 4.0, Release Branch 09.04, Trunk
> Reporter: Philippe Mouawad
> Assignee: Jacopo Cappellato
> Priority: Critical
> Fix For: Release Branch 10.04, Release Branch 11.04, Trunk
>
> Attachments: OFBIZ-2353 SELECT FOR UPDATE solution.patch, OFBIZ-2353
> SELECT FOR UPDATE solution.patch
>
>
> If Ofbiz is deploy on 2 servers in Load Balancing Mode
> SequenceUtil will generate duplicate IDs because synchronization is done at
> JVM level instead of doing it in DB.
> A good replacement implementation would be:
> org.hibernate.id.enhanced.TableGenerator
> But it would involve a dependency on Hibernate
> Philippe
> www.ubik-ingenierie.com
--
This message was sent by Atlassian JIRA
(v6.2#6252)
