[storm] probable bug in storm/0.19/storm.twisted.transact causes high db locks.

[Giovanni `evilaliv3` Pellerano]

2013/1/21 Free Ekanayaka <free.ekanayaka at gmail.com>:
> Hi Giovanni,
>
> thanks for reporting this.
>
> On Sat, Jan 19, 2013 at 2:01 PM, Giovanni `evilaliv3` Pellerano
> <evilaliv3 at globaleaks.org> wrote:
>> While debugging some code of the Globaleaks software i've probably
>> found a bug related to storm integration with twisted.
>>
>> in reference to storm/0.19/storm.twisted.transact, the
>> time.sleep(seconds) inside @transact decorator causes a long db lock
>> in case of exceptions.
>
> I'm not sure to understand what you mean exactly. The code itself
> seems fine (but see below), as it aborts the transaction before
> sleeping, so it won't cause a "db lock" (unless there's something
> weird going on with your user code or with sqlite).
>
> On the other hand I do see how this behavior is not
> performance-friendly for scenarios with a lot of concurrent requests
> AND database contention at the same time, because that sleep() will
> cause the thread to hang and other requests will have to wait.
>
>> in my opinion, the retry mechanism needs to be implemented outside of
>> the @transact method and not vice versa.        furthermore a sleep
>> expressed in seconds is probably too long for a transaction retry.
>
> This feature is indeed weird, it was put there for compatibility with
> some Zope code. It should be probably taken out of @transact and put
> in a different decorator, like you suggest, which ideally releases the
> thread and uses reactor.callLater instead of a blocking sleep. I think
> I'll try that.
>
> I also agree that sleeps should be more in the order of milliseconds
> than seconds.
>
>> here are some reference to the issue and to the temporary fix:
>> https://github.com/globaleaks/GLBackend/issues/44
>> https://github.com/globaleaks/GLBackend/commit/aec7e6902ed64ba33f856ca6bd69aa049a6cf64d
>
> The workaround that you put in place seems good, and it's actually not
> even a workaround, as the number of retries is configurable, so
> setting it to zero is perfectly acceptable. Probably zero should have
> been the default, to avoid surprises like this. My bad, I'm sorry that
> it caused you troubles.
>
> Cheers,
>
> Free

Thank you Free for your fast reply.
No problem exists at all =)

We will probably use a custom dectorator to handle custom handling of
exceptions.

Cheers,

Giovanni