[Ur] missing transactions (was Code review request)

[Sergey Mironov]

The following patch fixes the issue for me

https://raw.githubusercontent.com/grwlf/urweb-callback/d11bea0f03c03ceddbb5cf2183119ba8e798c49f/4_of_4_Tweak_transactional_callabcks.patch

Latest urweb-callback survives the stress test with this patch applied.

Regards,
Sergey


2014-04-10 17:00 GMT+04:00 Sergey Mironov <grrwlf at gmail.com>:
> Hi again! Looks like there are no people around who are brave enough
> to look into my code :)) But that's alright: after deep debugging I've
> fixed the segfaults (my mistake). And it is not the end of story:
>
> I've run a stress testing and it shows that some database transactions
> just disappear . Investigation shows an interesting point in uw_commit
> (see the code below). The code says that it is possible to return from
> the [uw_commit] function after calling all the
> transactionals[i].commit handlers (return 1 after db_commit). I think
> it is clearly a bug, since the user applications (like urweb-callback)
> think that everything is OK at this point. I've done some testing and
> it shows that this returns really happen from time to time under high
> load.
>
> So there are two questions:
>
>  1. What should application do if database commit fails? Probably, the
> answer is - report the error to client.
>  2. How to notify the ctx->transactionals about successful db_commit?
> Looks like we can't do that at the moment.  Here I suggest rewriting
> the code: call NULL-rolllbacked commits _after_ the db_commit point
> (and forbid raising errors for such handlers). This way we can really
> handle the non-transactional actions.
>
> Please, comment.
>
> Regards,
> Sergey
>
>
> --
>
> urweb.c   line 3291
>
>   for (i = ctx->used_transactionals-1; i >= 0; --i)
>     if (ctx->transactionals[i].rollback != NULL)
>       if (ctx->transactionals[i].commit) {
>         ctx->transactionals[i].commit(ctx->transactionals[i].data);
>         if (uw_has_error(ctx)) {
>           uw_rollback(ctx, 0);
>           return 0;
>         }
>       }
>
>   for (i = ctx->used_transactionals-1; i >= 0; --i)
>     if (ctx->transactionals[i].rollback == NULL)
>       if (ctx->transactionals[i].commit) {
>         ctx->transactionals[i].commit(ctx->transactionals[i].data);
>         if (uw_has_error(ctx)) {
>           uw_rollback(ctx, 0);
>           return 0;
>         }
>       }
>
>   if (ctx->transaction_started) {
>     int code = ctx->app->db_commit(ctx);
>
>     if (code) {
>       if (code == -1)
>         return 1;  // <--- HERE !
>
>       for (i = ctx->used_transactionals-1; i >= 0; --i)
>         if (ctx->transactionals[i].free)
>           ctx->transactionals[i].free(ctx->transactionals[i].data, 0);
>
>       uw_set_error_message(ctx, "Error running SQL COMMIT");
>       return 0;
>     }
>   }
>
>
>
> 2014-04-07 0:50 GMT+04:00 Sergey Mironov <grrwlf at gmail.com>:
>> Probably I should mention this thing: the notifiers::init() from
>> CallbackFFI.cpp:377 is called by the `task initialize' in the
>> Callback.ur
>>
>> https://github.com/grwlf/urweb-callback/blob/devel/Callback.ur#L18
>>
>>
>> Sergey