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

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


On June 8th, 2013, 7:03 p.m. UTC, Simeon Bird wrote:

Review request for Soprano and Vishesh Handa.
By Simeon Bird.

Updated June 8, 2013, 7:03 p.m.

Description

commit a7e2643a0fd6bd3608bafc6ea972b4b5d143e0a7
Author: Simeon Bird <spb@ias.edu>
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 <spb@ias.edu>
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

Testing

Compiled, ran
Bugs: 269442, 320123

Diffs

  • soprano/error.cpp (bff34dd)
  • soprano/error.h (12d233e)

View Diff