Re: [Soprano-devel] Review Request 110897: Improve soprano error handling

[Vishesh H. <me...@vh...>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110897/#review33956
-----------------------------------------------------------


Yes for (1), no for (2).

And maybe no for (1). Ideally (1) should be fixed by qDebug not printing these messages when Soprano is compiled in release mode or something like that. I remember reading a bug report about this on bugs.kde.org by Thiago. Maybe you should take a look at that. Otherwise commit (1), I guess.

I'm really not too fond of the ErrorCache's design because doing this hash lookup is not especially cheap, specially when it is done all over. That being said, it makes sense in the way it is being used in Soprano. Lets take the example of the Soprano::Model -

Two different threads can access two member functions of the same model and can return errors.

Thread 1 - Soprano::Model::executeQuery( Query1 );
Thread 2 - Soprano::Model::executeQuery( Query2 );

Thread 1 will want to check the errors, and so will thread 2, if both of these operations are happening somewhere around the same time, Thread1 could get Thread2's error and vice versa.

That being said - we need to find a way that the QHash does not just keep growing. I'm open to suggestions.

PS: Nice work with your patches!

- Vishesh Handa


On June 8, 2013, 7:03 p.m., Simeon Bird wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110897/
> -----------------------------------------------------------
> 
> (Updated June 8, 2013, 7:03 p.m.)
> 
> 
> Review request for Soprano and Vishesh Handa.
> 
> 
> Description
> -------
> 
> commit a7e2643a0fd6bd3608bafc6ea972b4b5d143e0a7
> Author: Simeon Bird <sp...@ia...>
> Date:   Sat Jun 8 14:16:29 2013 -0400
> 
>     Do not print the last error that soprano saw to the console. All this
>     does is fill up .xsession-errors. The caller can print the message
>     themselves if they want.
>     
>     BUG: 269442
> 
> commit 0f9dc7a6f434efd58ed21c99bc1cf280c2fb6e4e
> Author: Simeon Bird <sp...@ia...>
> Date:   Sat Jun 8 13:58:01 2013 -0400
> 
>     Soprano::Error::ErrorCache stores a QHash <QThread*, Error>
>     giving the last error in each thread.
>     
>     Once a thread has exited, there is no way to access or modify its error.
>     Since all actual work is done inside temporary threads, this is
>     not a sensible thing to do, and means that over time the QHash can
>     grow an infinite number of buckets, until it crashes.
>     
>     What the class seems actually meant to do is to pass error messages from
>     temporary threads back to the caller. Therefore, remove the per-thread
>     hash and just store the last error that occurred for this object
>     (ie, make it a per-object errno).
>     
>     BUG: 320123
> 
> 
> This addresses bugs 269442 and 320123.
>     http://bugs.kde.org/show_bug.cgi?id=269442
>     http://bugs.kde.org/show_bug.cgi?id=320123
> 
> 
> Diffs
> -----
> 
>   soprano/error.cpp bff34dd 
>   soprano/error.h 12d233e 
> 
> Diff: http://git.reviewboard.kde.org/r/110897/diff/
> 
> 
> Testing
> -------
> 
> Compiled, ran
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>