[transaction] Duplicate TX id's

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[transaction] Duplicate TX id's

David J. M. Karlsen
Hi!

I keep getting ResourceManagerExceptions with "Duplicate transaction
id". I took a look at the implementation of FileResourceManager - and
generatedUniqueID only returns system.getCurrentMillis() - which isn't
random enough in a multithreaded environment.

This should probably be changed into a UUID algorithm or something.

Also - the LoggerInterface has a createLogger method - but this is an
instance method - so the logging stuff should probably be changed into a
factory that creates logger of a specified type?

I also miss a LoggerFacade for jakarta's own Commons Logging (which
would make the other classes redundant).

Anyone?


--
David J. M. Karlsen - +47 90 68 22 43
http://www.davidkarlsen.com
http://mp3.davidkarlsen.com

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [transaction] Duplicate TX id's

Oliver Zeigermann
On 5/20/05, David J. M. Karlsen <[hidden email]> wrote:
> Hi!
>
> I keep getting ResourceManagerExceptions with "Duplicate transaction
> id". I took a look at the implementation of FileResourceManager - and
> generatedUniqueID only returns system.getCurrentMillis() - which isn't
> random enough in a multithreaded environment.

Hmmm, I wonder why this is so? Looking at generatedUniqueTxId() it
checks if the transaction Id already is in use by

            } while (getContext(txId) != null);

Are you sure this is the reason for your exceptions?

> This should probably be changed into a UUID algorithm or something.

This would be a good idea in any case :)

> Also - the LoggerInterface has a createLogger method - but this is an
> instance method - so the logging stuff should probably be changed into a
> factory that creates logger of a specified type?

Would be cleaner, yes.

> I also miss a LoggerFacade for jakarta's own Commons Logging (which
> would make the other classes redundant).

Not quite. Commons Logging does not map to the Jakarta Slide logging
system which makes heavy use of commons transaction. That's why there
is such a LoggerFacade. Aside, a Commons logging implementation would
be a good idea :)

> Anyone?

If you could propose any patches I would be happy to consider them ;)

Oliver

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [transaction] Duplicate TX id's

David J. M. Karlsen
Oliver Zeigermann wrote:

>On 5/20/05, David J. M. Karlsen <[hidden email]> wrote:
>  
>
>>Hi!
>>
>>I keep getting ResourceManagerExceptions with "Duplicate transaction
>>id". I took a look at the implementation of FileResourceManager - and
>>generatedUniqueID only returns system.getCurrentMillis() - which isn't
>>random enough in a multithreaded environment.
>>    
>>
>
>Hmmm, I wonder why this is so? Looking at generatedUniqueTxId() it
>checks if the transaction Id already is in use by
>
>            } while (getContext(txId) != null);
>
>Are you sure this is the reason for your exceptions?
>  
>
Hmm, yeah, but still - I cannot see why txId should be unique, the code is:

public String generatedUniqueTxId() throws ResourceManagerSystemException {
        assureRMReady();
        String txId;
        synchronized (globalTransactions) {
            do {
                txId = Long.toHexString(System.currentTimeMillis);
                // XXX busy loop
            } while (getContext(txId) != null);
        }
        return txId;
    }


you can apply my patch.
It uses doomdark.org 's UUIDGenerator. (the site is down at the moment).

>>This should probably be changed into a UUID algorithm or something.
>>    
>>
>
>This would be a good idea in any case :)
>
>  
>
>>Also - the LoggerInterface has a createLogger method - but this is an
>>instance method - so the logging stuff should probably be changed into a
>>factory that creates logger of a specified type?
>>    
>>
>
>Would be cleaner, yes.
>
>  
>
>>I also miss a LoggerFacade for jakarta's own Commons Logging (which
>>would make the other classes redundant).
>>    
>>
>
>Not quite. Commons Logging does not map to the Jakarta Slide logging
>system which makes heavy use of commons transaction. That's why there
>is such a LoggerFacade. Aside, a Commons logging implementation would
>be a good idea :)
>
>  
>
>>Anyone?
>>    
>>
>
>If you could propose any patches I would be happy to consider them ;)
>  
>
I'll have a look into the logging as well.

Index: FileResourceManager.java
===================================================================
RCS file: /home/cvspublic/jakarta-commons/transaction/src/java/org/apache/commons/transaction/file/FileResourceManager.java,v
retrieving revision 1.6
diff -u -r1.6 FileResourceManager.java
--- FileResourceManager.java 7 Jan 2005 13:32:33 -0000 1.6
+++ FileResourceManager.java 21 May 2005 16:07:22 -0000
@@ -42,6 +42,10 @@
 import java.util.Iterator;
 import java.util.Collections;
 
+import org.doomdark.uuid.UUIDGenerator;
+import org.doomdark.uuid.UUID;
+
+
 import org.apache.commons.transaction.locking.GenericLock;
 import org.apache.commons.transaction.locking.GenericLockManager;
 import org.apache.commons.transaction.locking.LockException;
@@ -881,7 +885,9 @@
         String txId;
         synchronized (globalTransactions) {
             do {
-                txId = Long.toHexString(System.currentTimeMillis());
+                //txId = Long.toHexString(System.currentTimeMillis());
+             UUID uuid = UUIDGenerator.getInstance().generateRandomBasedUUID();
+             txId = uuid.toString();
                 // XXX busy loop
             } while (getContext(txId) != null);
         }


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [transaction] Duplicate TX id's

Oliver Zeigermann
On 5/23/05, David J. M. Karlsen <[hidden email]> wrote:

> Oliver Zeigermann wrote:
> > On 5/21/05, David J. M. Karlsen <[hidden email]> wrote:
> >
> >>Hmm, yeah, but still - I cannot see why txId should be unique, the code is:
> >>
> >>public String generatedUniqueTxId() throws ResourceManagerSystemException {
> >>        assureRMReady();
> >>        String txId;
> >>        synchronized (globalTransactions) {
> >>            do {
> >>                txId = Long.toHexString(System.currentTimeMillis);
> >>                // XXX busy loop
> >>            } while (getContext(txId) != null);
> >>        }
> >>        return txId;
> >>    }
> >>
> >
> >
> > It will if you actually use the id to start a new transaction. In this
> > case getContext(txId)  will not return null and the procedure will be
> > repeated.
> It only ensures that you are not given a txId *in use* (eg. started).
> Several equal txId maybe produced by this method - and worse - you will
> only discover this when you try to start the supposedly unique TX. Thus,
> it is broken... It should, as the name implies, generate truly unique id's.

I disagree. The name implies that it generates a truly unique *tx* id,
i.e. a tx id not currently used by any transaction which it does.
 

> >>you can apply my patch.
> >>It uses doomdark.org 's UUIDGenerator. (the site is down at the moment).
> >
> >
> > I would prefer if you simply generated your ids using this generator
> > without changing the file resource manager code for two reasons:
> that's what I'm doing in my app. (Providing the ID's myself, and not
> calling generateUniqueID).
>
> >
> > (1) The above code is not broken
> I beg to differ.
>
> > (2) Changing the code would introduce a new dependency to another lib
> True, that is bad. I'll put the generator code into another method that
> we may call. OK?

Depends on the original license of the code.

> > (3) The lib's licencse is unclear to me
> I saw it included in some sandbox jakarta code - but that does not
> neccessarily mean the license is OK.

I could not even check what license it has. Which on is it?

Still not convinced ;)

Oliver

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [transaction] Duplicate TX id's

David J. M. Karlsen
Oliver Zeigermann wrote:

>>>>Hmm, yeah, but still - I cannot see why txId should be unique, the code is:
>>>>
>>>>public String generatedUniqueTxId() throws ResourceManagerSystemException {
>>>>       assureRMReady();
>>>>       String txId;
>>>>       synchronized (globalTransactions) {
>>>>           do {
>>>>               txId = Long.toHexString(System.currentTimeMillis);
>>>>               // XXX busy loop
>>>>           } while (getContext(txId) != null);
>>>>       }
>>>>       return txId;
>>>>   }
>>>>
>>>>        
>>>>
>>>It will if you actually use the id to start a new transaction. In this
>>>case getContext(txId)  will not return null and the procedure will be
>>>repeated.
>>>      
>>>
>>It only ensures that you are not given a txId *in use* (eg. started).
>>Several equal txId maybe produced by this method - and worse - you will
>>only discover this when you try to start the supposedly unique TX. Thus,
>>it is broken... It should, as the name implies, generate truly unique id's.
>>    
>>
>
>I disagree. The name implies that it generates a truly unique *tx* id,
>i.e. a tx id not currently used by any transaction which it does.
>  
>
That may be so - but don't you think it's more practical to get a really
unique UUID (which still is guaranteed not to be in use) - opposed to
getting a id which *may* be unique (and still guaranteed to not be in use).
Which means the tx is rolled back - you have to catch the exception -
parse the string, find that it wasn't unique and generating another one
- and hoping for it not to be in use? It would be possible to return the
same string each time (an extreme case/example) - it would still be
unique - but only the one txId could be running at any point in time.
(effectively making transactions redundant :-)) I think so, so I don't
use the one from the API (as it very often collides with one already in
use) - but have to provide them myself.

>(1) The above code is not broken
>  
>
>>I beg to differ.
>>
>>    
>>
>>>(2) Changing the code would introduce a new dependency to another lib
>>>      
>>>
>>True, that is bad. I'll put the generator code into another method that
>>we may call. OK?
>>    
>>
>
>Depends on the original license of the code.
>  
>
Yeah - maybe we could pull out parts of commons ID until it is out of
the sandbox?

>>>(3) The lib's licencse is unclear to me
>>>      
>>>
>>I saw it included in some sandbox jakarta code - but that does not
>>neccessarily mean the license is OK.
>>    
>>
>I could not even check what license it has. Which on is it?
>  
>
It's used here:
http://ws.apache.org/ws-fx/addressing/xref/org/apache/ws/addressing/uuid/JugUUIdGenerator.html
But if we should depend on anything - commons ID would be the right
thing (TM) - but it's still in the sandbox.


>Still not convinced ;)
>  
>
:-) I understand the licencing and depdendency issues - but still, I
truly belive generateUniqeTXID should return a truly random ID - and I
would guess that almost every transaction manager made would work that way.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]