Skip Menu |
 

This queue is for tickets about the UUID-Tiny CPAN distribution.

Report information
The Basics
Id: 57188
Status: open
Priority: 0/
Queue: UUID-Tiny

People
Owner: CAUGUSTIN [...] cpan.org
Requestors: mschwern [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: 1.03
Fixed in: (no value)

Attachments
0001-Choose-the-thread-safe-Digest-SHA-PurePerl-if-thread.patch
0001-Make-UUID-Tiny-mostly-work-with-threads.patch



Subject: Partial thread support
Download (untitled) / with headers
text/plain 970b
UUID::Tiny doesn't work with threads. The main problem is you can't call lock on an unshared variable, you have to declare it shared first. By default, anything not declared shared is copied per thread (this is far safer than the traditional threading model). It might save a little bit of performance to not copy the Digest objects, but its not necessary and sharing objects gets annoying, especially XS ones. $Last_Pid should not be shared across threads else if two threads fork they'll clobber each other. $Clk_Seq and $Last_Timestamp, I think, should be shared. Otherwise each thread would have its own $Clk_Seq and increment it in parallel presumably winding up with the same value. With the attached patch in place, threading works... sort of. The problem is Digest::SHA and Digest::SHA1 do not appear to be thread safe. Creating one of their objects causes a lot of malloc warnings, but the basic test passes. Digest::SHA::PurePerl has no such problem.
Subject: 0001-Make-UUID-Tiny-mostly-work-with-threads.patch
From 747483a74dba588cf3d9d0c5c88fbcab0b8886c7 Mon Sep 17 00:00:00 2001 From: Michael G. Schwern <schwern@pobox.com> Date: Mon, 3 May 2010 18:11:35 -0700 Subject: [PATCH] Make UUID::Tiny mostly work with threads. Digest::SHA and Digest::SHA1 have threading bugs. --- lib/UUID/Tiny.pm | 13 ++++--------- t/threads.t | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 9 deletions(-) create mode 100644 t/threads.t diff --git a/lib/UUID/Tiny.pm b/lib/UUID/Tiny.pm index 5a7623d..4954d0a 100644 --- a/lib/UUID/Tiny.pm +++ b/lib/UUID/Tiny.pm @@ -9,7 +9,7 @@ use MIME::Base64; use Time::HiRes; use POSIX; -our $SHA1_CALCULATOR = undef; +my $SHA1_CALCULATOR = undef; { # Check for availability of SHA-1 ... @@ -22,7 +22,7 @@ our $SHA1_CALCULATOR = undef; }; }; -our $MD5_CALCULATOR = Digest::MD5->new(); +my $MD5_CALCULATOR = Digest::MD5->new(); # ToDo: @@ -388,8 +388,6 @@ sub _create_v3_uuid { my $name = shift; my $uuid = ''; - lock $MD5_CALCULATOR; - # Create digest in UUID ... $MD5_CALCULATOR->reset(); $MD5_CALCULATOR->add($ns_uuid); @@ -441,8 +439,6 @@ sub _create_v5_uuid { ; } - lock $SHA1_CALCULATOR; - $SHA1_CALCULATOR->reset(); $SHA1_CALCULATOR->add($ns_uuid); @@ -681,7 +677,7 @@ sub equal_uuids { # Private functions ... # my $Last_Pid; -my $Clk_Seq; +my $Clk_Seq :shared; # There is a problem with $Clk_Seq and rand() on forking a process using # UUID::Tiny, because the forked process would use the same basic $Clk_Seq and @@ -690,7 +686,6 @@ my $Clk_Seq; # time before using $Clk_Seq or rand() ... sub _init_globals { - lock $Last_Pid; lock $Clk_Seq; if (!defined $Last_Pid || $Last_Pid != $$) { @@ -714,7 +709,7 @@ sub _init_globals { return; } -my $Last_Timestamp; +my $Last_Timestamp : shared; sub _get_clk_seq { my $ts = shift; diff --git a/t/threads.t b/t/threads.t new file mode 100644 index 0000000..139e3c5 --- /dev/null +++ b/t/threads.t @@ -0,0 +1,39 @@ +#!/usr/bin/perl + +use strict; +use warnings; + +my $Have_Threads; +BEGIN { + $Have_Threads = eval { + require threads; + require threads::shared; + threads::shared->import; + 1; + }; + require Test::More; + Test::More->import; +} + +plan skip_all => "Needs threads" unless $Have_Threads; + +use UUID::Tiny qw(:std); + +my $Num_Threads = 5; + +my %uuids : shared; +$uuids{create_uuid_as_string(UUID_V4)}++; + +for(1..$Num_Threads) { + threads->create(sub { + $uuids{create_uuid_as_string(UUID_V4)}++; + }); +} + +note "All threads started"; +$_->join for threads->list; +note "All threads joined"; + +is keys %uuids, $Num_Threads + 1, "All v4 uuids should be unique per thread"; + +done_testing; -- 1.7.0.3
Subject: Re: [rt.cpan.org #57188] Partial thread support
Date: Tue, 4 May 2010 18:39:22 +0200
To: bug-UUID-Tiny [...] rt.cpan.org
From: Christian Augustin <cma [...] snafu.de>
Download (untitled) / with headers
text/plain 2.4k
Thanks for your support and the tips. I applied your patches, but I can't get the test running - I constantly get the following error message under Mac OS X 10.6.3 (with varying numbers): t/04-UUID-threads.t .. # All threads started perl(711,0x7fff70193be0) malloc: *** error for object 0x1004e1420: pointer being freed was not allocated *** set a breakpoint in malloc_error_break to debug I changed "note" to "diag" to see were things go bad. Seems to be a problem when a thread is joined; when I commented the line (I know, that's bad :-), I've gotten different results ... I can't reproduce them now, but I eventually got only 5 instead of 6 UUIDs in the hash (and I constantly get a message complaining that Perl exited with active threads, but I guessed something like that would happen :-). I have no idea how to investigate in the error message above and how to do the debugging to see what's wrong. Poking around in the innards of Perl is beyond my horizon, so any help is appreciated. Is there a specific project that made you look into UUID::Tiny, or just out of curiosity? Kind regards -- Christian Augustin Am 04.05.2010 um 03:13 schrieb Michael G Schwern via RT: Show quoted text
> Mon May 03 21:13:41 2010: Request 57188 was acted upon. > Transaction: Ticket created by MSCHWERN > Queue: UUID-Tiny > Subject: Partial thread support > Broken in: 1.03 > Severity: Normal > Owner: Nobody > Requestors: mschwern@cpan.org > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=57188 > > > > UUID::Tiny doesn't work with threads. The main problem is you can't > call lock on an unshared variable, you have to declare it shared first. > By default, anything not declared shared is copied per thread (this is > far safer than the traditional threading model). > > It might save a little bit of performance to not copy the Digest > objects, but its not necessary and sharing objects gets annoying, > especially XS ones. > > $Last_Pid should not be shared across threads else if two threads fork > they'll clobber each other. > > $Clk_Seq and $Last_Timestamp, I think, should be shared. Otherwise each > thread would have its own $Clk_Seq and increment it in parallel > presumably winding up with the same value. > > With the attached patch in place, threading works... sort of. The > problem is Digest::SHA and Digest::SHA1 do not appear to be thread safe. > Creating one of their objects causes a lot of malloc warnings, but the > basic test passes. Digest::SHA::PurePerl has no such problem.
Subject: Re: [rt.cpan.org #57188] Partial thread support
Date: Tue, 04 May 2010 14:02:21 -0700
To: bug-UUID-Tiny [...] rt.cpan.org
From: Michael G Schwern <schwern [...] pobox.com>
Download (untitled) / with headers
text/plain 2.3k
On 2010.5.4 9:39 AM, Christian Augustin via RT wrote: Show quoted text
> Thanks for your support and the tips. I applied your patches, but I can't get the test running - I constantly get the following error message under Mac OS X 10.6.3 (with varying numbers): > > t/04-UUID-threads.t .. # All threads started > perl(711,0x7fff70193be0) malloc: *** error for object 0x1004e1420: pointer being freed was not allocated > *** set a breakpoint in malloc_error_break to debug
Yes, that's the problem coming from Digest::SHA or Digest::SHA1 that I mentioned. The test demonstrates the problem. Show quoted text
> I changed "note" to "diag" to see were things go bad. Seems to be a problem when a thread > is joined; when I commented the line (I know, that's bad :-), I've gotten
different Show quoted text
> results ... I can't reproduce them now, but I eventually got only 5 instead
of 6 UUIDs Show quoted text
> in the hash (and I constantly get a message complaining that Perl exited
with active threads, Show quoted text
> but I guessed something like that would happen :-).
That's expected behavior. If you don't wait for the threads to finish (that's what join does) then the test might check the hash before the threads are done populating it. Show quoted text
> I have no idea how to investigate in the error message above and how to do the debugging > to see what's wrong. Poking around in the innards of Perl is beyond my
horizon, so any Show quoted text
> help is appreciated.
I see two options. First, hope that Digest::SHA1 fixes threading. I've reported it. https://rt.cpan.org/Ticket/Display.html?id=57189 Or if UUID::Tiny sees that threads are loaded, it can use Digest::SHA::PurePerl. This should work so long as threads are turned on before UUID::Tiny is loaded. The attached patch implements that on top of my last one. There is an issue that Digest::MD5 throws some warnings during global destruction. I'm not sure what that's about, you could just suppress it. You may want to decide how far down the rabbit hole you want to go with this. Show quoted text
> Is there a specific project that made you look into UUID::Tiny, or just out of curiosity?
Object::ID needs a thread-safe UUID generator for its object_uuid() method. Data::UUID is not thread-safe. -- 31. Not allowed to let sock puppets take responsibility for any of my actions. -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army http://skippyslist.com/list/

Message body is not shown because sender requested not to inline it.

Subject: Re: [rt.cpan.org #57188] Partial thread support
Date: Wed, 5 May 2010 07:43:12 +0200
To: bug-UUID-Tiny [...] rt.cpan.org
From: Christian Augustin <cma [...] snafu.de>
Am 04.05.2010 um 23:03 schrieb Michael G Schwern via RT: Show quoted text
> I see two options. First, hope that Digest::SHA1 fixes threading. I've > reported it. https://rt.cpan.org/Ticket/Display.html?id=57189 > > Or if UUID::Tiny sees that threads are loaded, it can use > Digest::SHA::PurePerl. This should work so long as threads are turned on > before UUID::Tiny is loaded. The attached patch implements that on top of my > last one. > > There is an issue that Digest::MD5 throws some warnings during global > destruction. I'm not sure what that's about, you could just suppress it. You > may want to decide how far down the rabbit hole you want to go with this. > >
>> Is there a specific project that made you look into UUID::Tiny, or just out of curiosity?
> > Object::ID needs a thread-safe UUID generator for its object_uuid() method. > Data::UUID is not thread-safe.
I see. Give me some time (I'm busy at the moment) and I'll think about it. Perhaps we can find an appropriate solution. -- Christian Augustin
Subject: Re: [rt.cpan.org #57188] Partial thread support
Date: Tue, 04 May 2010 23:11:23 -0700
To: bug-UUID-Tiny [...] rt.cpan.org
From: Michael G Schwern <schwern [...] pobox.com>
Download (untitled) / with headers
text/plain 873b
On 2010.5.4 10:43 PM, Christian Augustin via RT wrote: Show quoted text
>>> Is there a specific project that made you look into UUID::Tiny, or just out of curiosity?
>> >> Object::ID needs a thread-safe UUID generator for its object_uuid() method. >> Data::UUID is not thread-safe.
> > I see. Give me some time (I'm busy at the moment) and I'll think about it. > Perhaps we can find an appropriate solution.
rafl is working on making the Digest modules thread safe. Hopefully, if we wait a few days, my second patch will not be necessary. Also, Object::ID strictly doesn't *need* a thread-safe UUID generator. It would just be nice if using Object::ID didn't mean you couldn't use threads. There's no urgency. -- 125. Two drink limit does not mean two kinds of drinks. -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army http://skippyslist.com/list/
Subject: Re: [rt.cpan.org #57188] Partial thread support
Date: Wed, 5 May 2010 10:41:29 +0200
To: bug-UUID-Tiny [...] rt.cpan.org
From: Christian Augustin <cma [...] snafu.de>
Download (untitled) / with headers
text/plain 729b
Am 05.05.2010 um 08:11 schrieb Michael G Schwern via RT: Show quoted text
> rafl is working on making the Digest modules thread safe. Hopefully, if we > wait a few days, my second patch will not be necessary.
It would be necessary, because UUID::Tiny could be used on systems without the latest patches, so it should be able to cope with such situations. Show quoted text
> Also, Object::ID strictly doesn't *need* a thread-safe UUID generator. It > would just be nice if using Object::ID didn't mean you couldn't use threads. > There's no urgency.
Yes, that would be nice. Same with UUID::Tiny. So I have to think about the associated problems. I'll come back to you with some thoughts and proposals (by end of this week, I think). -- Christian Augustin
Subject: Re: [rt.cpan.org #57188] Partial thread support
Date: Mon, 14 Jun 2010 12:42:04 +0200
To: bug-UUID-Tiny [...] rt.cpan.org
From: Christian Augustin <cma [...] snafu.de>
Download (untitled) / with headers
text/plain 323b
Is there a way to get information about when the used Digest modules become thread-safe and the respective version numbers? I would like to incorporate tests to explicitly decide when a Digest module can be used with threads and to provide some way of checking and error reporting with UUID::Tiny. -- Christian Augustin
Download (untitled) / with headers
text/plain 407b
On Mon Jun 14 06:42:18 2010, cma@snafu.de wrote: Show quoted text
> Is there a way to get information about when the used Digest modules > become thread-safe and the respective version numbers? I would like > to incorporate tests to explicitly decide when a Digest module can > be used with threads and to provide some way of checking and error > reporting with UUID::Tiny.
I'd just read their change log files.
Download (untitled) / with headers
text/plain 183b
Can't get this to work properly, this is far beyond me. Cancelled test coverage for threads too because I can't get rid of the errors. So: No reliable thread support with UUID::Tiny!
Download (untitled) / with headers
text/plain 629b
On Mon Aug 19 10:15:56 2013, CAUGUSTIN wrote: Show quoted text
> Can't get this to work properly, this is far beyond me. Cancelled test > coverage for threads too because I can't get rid of the errors. > > So: No reliable thread support with UUID::Tiny!
i haven't digested all of this, nor what i'm about to post, but what about stealing some ideas from: http://cpansearch.perl.org/src/JESSE/Prophet-0.71/lib/Prophet/TempUUIDTiny.pm they claim it's thread-safe, maybe there's a nugget or two of intel in there that we can use. i'm adding this to my TODO list to look through, since Tiny::UUID looks promising long term (OSSP::uuid is hard).
Subject: Re: [rt.cpan.org #57188] Partial thread support
Date: Tue, 19 Nov 2013 12:21:54 +0100
To: bug-UUID-Tiny [...] rt.cpan.org
From: Christian Augustin <cma [...] snafu.de>
Download (untitled) / with headers
text/plain 1.5k
Hi Wes, I have to do a diff, but I think this is all incorporated in my current version of UUID::Tiny (Jesse helped me to fix some problems, and this looks like an intermediate version). The threading problem is in the Perl libraries (Digest modules), not in UUID::Tiny itself. As far as I know, at least. But because UUID::Tiny uses those modules I can’t say that it will reliably work with threading. I’m not even able to do proper testing, threading is completely out of my realm of expertise and occurring errors are complete gobbledygook to me. If I can have a solid solution *including* reliable test coverage I will have a look on it and gladly incorporate it into UUID::Tiny if I can get it to work. No luck so far. Best -- Christian Augustin Am 2013-11-19 um 12:03 schrieb "Wes Young via RT" <bug-UUID-Tiny@rt.cpan.org>: Show quoted text
> Queue: UUID-Tiny > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=57188 > > > On Mon Aug 19 10:15:56 2013, CAUGUSTIN wrote:
>> Can't get this to work properly, this is far beyond me. Cancelled test >> coverage for threads too because I can't get rid of the errors. >> >> So: No reliable thread support with UUID::Tiny!
> > > i haven't digested all of this, nor what i'm about to post, but what about stealing some ideas from: > > http://cpansearch.perl.org/src/JESSE/Prophet-0.71/lib/Prophet/TempUUIDTiny.pm > > they claim it's thread-safe, maybe there's a nugget or two of intel in there that we can use. > > i'm adding this to my TODO list to look through, since Tiny::UUID looks promising long term (OSSP::uuid is hard).
Subject: Re: [rt.cpan.org #57188] Partial thread support
Date: Tue, 19 Nov 2013 15:30:11 +0100
To: "bug-UUID-Tiny [...] rt.cpan.org" <bug-UUID-Tiny [...] rt.cpan.org>
From: Christian Augustin <cma [...] snafu.de>
It could be that UUID::Tiny behaves well with threading if only used to generate time based and random UUIDs, because MD5/SHA1 and therefor the Digest modules are not used for those. I can’t tell. Best -- Christian Augustin Am 19.11.2013 um 12:03 schrieb "Wes Young via RT" <bug-UUID-Tiny@rt.cpan.org>: Show quoted text
> Queue: UUID-Tiny > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=57188 > > > On Mon Aug 19 10:15:56 2013, CAUGUSTIN wrote:
>> Can't get this to work properly, this is far beyond me. Cancelled test >> coverage for threads too because I can't get rid of the errors. >> >> So: No reliable thread support with UUID::Tiny!
> > > i haven't digested all of this, nor what i'm about to post, but what about stealing some ideas from: > > http://cpansearch.perl.org/src/JESSE/Prophet-0.71/lib/Prophet/TempUUIDTiny.pm > > they claim it's thread-safe, maybe there's a nugget or two of intel in there that we can use. > > i'm adding this to my TODO list to look through, since Tiny::UUID looks promising long term (OSSP::uuid is hard).
Download (untitled) / with headers
text/plain 496b
On Tue Nov 19 09:30:31 2013, cma@snafu.de wrote: Show quoted text
> It could be that UUID::Tiny behaves well with threading if only used > to generate time based and random UUIDs, because MD5/SHA1 and therefor > the Digest modules are not used for those. I can’t tell. > > Best
lol, i just sat down to digest what i was looking at, and then realized you guys were cross-pollinating already. all i saw was the "this should be thread safe" but didn't rlz it was just your 1.01 ver of the mod :) my bad. thanks!
Download (untitled) / with headers
text/plain 301b
i'm looking at the Digest::SHA history and i think you're probably right. however, i commented out the Digest::SHA/1 and just left the ::PurePerl version and it *appears* to test OK? wanna double check me? seems too easy to be true here.. :) (perl threading is fun once you wrap your head around it)
Download (untitled) / with headers
text/plain 736b
diff --git a/lib/UUID/Tiny.pm b/lib/UUID/Tiny.pm index c3e38df..0a0931e 100644 --- a/lib/UUID/Tiny.pm +++ b/lib/UUID/Tiny.pm @@ -14,8 +14,8 @@ my $SHA1_CALCULATOR = undef; { # Check for availability of SHA-1 ... local $@; # don't leak an error condition - eval { require Digest::SHA; $SHA1_CALCULATOR = Digest::SHA->new(1) } || - eval { require Digest::SHA1; $SHA1_CALCULATOR = Digest::SHA1->new() } || + #eval { require Digest::SHA; $SHA1_CALCULATOR = Digest::SHA->new(1) } || + #eval { require Digest::SHA1; $SHA1_CALCULATOR = Digest::SHA1->new() } || eval { require Digest::SHA::PurePerl; $SHA1_CALCULATOR = Digest::SHA::PurePerl->new(1) @@ -24,7 +24,6 @@ my $SHA1_CALCULATOR = undef;


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.