Olly, I apologize if I sounded inflammatory because this was certainly not
my object.
I am going to try to clarify my issue. By the way, this is the exact same
issue as the one with std::string, about which there are numerous
discussions on the web. I am just going to cite an excerpt of an
interesting answer on stackoverflow, which expresses the issue better than
I might:
The user should always be responsible for synchronization if he/she wants
to share objects among different threads. The issue here is whether a
library component uses and shares some hidden data structures that might
lead to data races even if "functions are applied on different
objects"
from a user's perspective.
http://stackoverflow.com/questions/1594803/is-stdstring-thead-safe-with-gcc-4-3
My situation is the following: the first thread puts a *copy* of a
Xapian::Document object on a queue, then goes back to whatever it is
doing. The original Xapian::Document was a local variable so it's deleted
while the function returns.
The second thread gets at what seems to be a *copy* and peruses it.
The appearance of things is that there is no object sharing going on.
Sharing only occurs because of an implementation detail.
When I wrote the code, I took a look at the header file, and saw that the
data was shared between objects, so that I should take some care not to
modify the old object data once I had passed the copy. Looked at the code,
seemed ok.
Because I am not clever enough, I did not think of the reference count
updates.
I was encouraged in this error by the fact that std::string does protect
its refcounts (at least the gnu one does), so that I pass string copies all
the time and don't get punished (fixed this too by the way, "chat
?chaud?
craint l'eau froide" as we say in French: scalded cats fear cold
water).
I am not contesting that I was wrong, just claiming that this was a
reasonable error, which could be avoided by a small phrase in the header
file, stating very simply that "the reference counts are not mt-safe".
This should be enough to alert the user, having them deduct that you can't
transfer a copy of the object to another thread without taking quite
intrusive precautions.
In my case, it's just simpler to switch to heap-allocated objects and
pointers, and make clear that object ownership is transferred.
I am adding a few remarks inside your answer below, once again, not wanting
to fuel a polemic, just to clarify.
Olly Betts writes:
> On Mon, May 05, 2014 at 08:12:15AM +0200, Jean-Francois Dockes wrote:
> > Olly Betts writes:
> > > On Sun, May 04, 2014 at 08:16:50PM +0200, Jean-Francois Dockes
wrote:
> > > > While investigating very infrequent crashes in the Recoll
indexer, I have
> > > > come to a very basic question: is it safe to pass a copy
of a
> > > > Xapian::Document from thread to thread (multiple threads
queue documents,
> > > > other thread updates the index) ?
> > > >
> > > > I don't seem to get directly into trouble while doing
this, but I don't see
> > > > anything either in the RefCntr implementation which would
explicitely make
> > > > it thread-safe, so I am wondering. Maybe I'm just
missing the obvious.
> > >
> > >
http://getting-started-with-xapian.readthedocs.org/en/latest/concepts/concurrency.html
> >
> > This only warns about documents read from the index and sharing
Database
> > references. Mine are just created from external data.
>
> That part says "for example".
>
> These parts apply to your situation though:
>
> "Xapian doesn?t maintain any global state, so you can safely use
> Xapian in a multi-threaded program provided you don?t share objects
> between threads"
>
> And:
>
> "If you really want to access the same Xapian object from
multiple
> threads, then you need to ensure that it won?t ever be accessed
> concurrently (if you don?t ensure this bad things are likely to
> happen - for example crashes or even data corruption). One way to
> prevent concurrent access is to require that a thread gets an
> exclusive lock on a mutex while the access is made."
Ok. As stated above, the *appearance* of things in my code was that no
object sharing was going on.
> > Only mentionning the Xapian::Database reference problem in the
document
> > almost makes things worse as one is led to believe that it is the
main or
> > only issue.
>
> These implicit references are an important thing for the user to be
> aware of, so really need to be mentioned.
Yes, I am not suggesting that this should be removed, just maybe add a
mention to the reference count. Maybe an actual exemple of how things can
go wrong would be useful.
> > Not my case anyway, as I think that the code is older than the
> > document.
>
> Xapian has behaved this way since 0.5.0, which was released more than a
> decade ago. The document I linked to was indeed written more recently,
> but it just attempted to document the existing situation with threads in
> a place where new API users would see it.
I just meant that I could not have read the page before writing the code.
> > I am not requesting thread-safe counters, and I understand that
faking copy
> > semantics may be useful, but I think that unprotected reference
counters
> > should be warned about.
>
> I'm not really sure what you're hoping the text should say here.
Are
> you thinking that calling the copy constructor, assignment operator, etc
> don't count as accessing the object? We could certainly easy clarify
> that.
Yes, I think that a reminder would be useful, saying that these operators
are not thread-safe even in some situations where they would appear to be
(mostly against an implicit destruction), because the reference count
updates are unprotected.
> > This is specially true because there is nothing that the library user
can
> > do to mitigate the problem. Data accesses can be protected if you
want to
> > share these objects, but it would be difficult with the counters, and
the
> > only possibility is to use pointers, even in seemingly trivial cases.
>
> You just need a mutex which has to be held before a thread can affect
> the reference count. The simplest would be a "Xapian" mutex
which a
> thread has to hold before it can do anything with any Xapian object.
>
> > This is a documentation issue, please, add a word in the header
comments:
> > only pointers to Xapian::Document (and others) can hop threads.
>
> I don't think that really expresses the restrictions accurately.
>
> For example, if you have a std::list<Xapian::Document> structure and
> protect access to it with a mutex, you should be able to safely append
> new objects to it from the creating threads and process objects already
> in the list from your indexing threads. No explicit pointers to
> Xapian::Document are involved, though some may be behind the scenes.
>
> And if you only have a single static global Xapian::Document object,
> then you can add terms to it in one thread and then add it to a database
> in another thread - no pointers to Xapian::Document are involved at all.
>
> > I'm not sure if the base.h header comments speculating about the
atomicity
> > of pointer copying are a fine example of English humour of if they
were
> > placed to further confuse the investigator :)
>
> Those comments date back to the pre-0.5.0 days - they're just very out
> of date, and I've just removed them.
Thanks :)
Cheers,
jf