[Xapian-discuss] Xapian::Document and threads

[Jean-Francois Dockes]

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