Attribute handler table should not be thread-local

Bug #1042404 reported by Tamás Nepusz
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
igraph
Fix Released
High
Tamás Nepusz

Bug Description

Currently the attribute handler in src/attributes.c is defined as thread-local, which presents a problem when

a) an application using the igraph library is multi-threaded and different threads use different attribute handlers. In this case, graphs created in one thread are incompatible with graphs created in another. Pretty pathological case, but it is still a problem.

b) a Python application using the igraph library spawns a new thread and calls the Graph constructor from the new thread. Since the new thread does not have access to the Python attribute handler (and we cannot hook into the thread creation process to set the attribute handler there), the module crashes whenever an attempt is made to read the graph attributes.

There are two possible solutions that I can think of:

a) Make the attribute handler global again as it was in 0.5.x, and either protect it with mutexes (to maintain thread-safety) or add a big warning sign that asks the user to call it from the main thread only.

b) Add an extra pointer to igraph graphs that point to the attribute handlers they were created with. This would require some extra legwork when copying a graph because the copy has to inherit the same attribute handler. Probably not a big deal.

Revision history for this message
Gábor Csárdi (gabor.csardi) wrote :

As for solutions, I obviously don't like b), because it is too much work for probably a small number of users. So I would vote for a).

In the first round, it might be enough to solve problem b), i.e. the problem with the Python interface. Plus we can document that people attaching attribute handlers in multithreaded programs should attach an attribute handler from each thread that is calling igraph functions. (Bad, but there are not too many people doing this, I believe.)

Revision history for this message
Tamás Nepusz (ntamas) wrote :

Solving problem b) is not possible without reverting to a global attribute handler table (i.e. solution a), because AFAIK there is no way to catch when a new thread is created from Python in order to set the attribute handler there. I'm perfectly fine with solution a) though. I was wondering whether it would qualify as an API change or not (in other words, whether we should postpone it till 0.7).

Revision history for this message
Gábor Csárdi (gabor.csardi) wrote :

I don't think you need to catch the thread creation, you just need to (re)install the attribute handler, every time you call back to C. Not a big thing, it just sets a pointer, should be fast. Or maybe I am overlooking something.

As for the API change, I think it is fine for 0.6.1. The number of programs that work as intended with the current implementation, but will fail if we do solution a), should be very close to zero.

Revision history for this message
Tamás Nepusz (ntamas) wrote :

The only thing you are overlooking here is that I was (and am) still too lazy to switch to the interface generator you use for the R interface and ensuring that I re-install the attribute handler every time I call back to C would involve a lot of searching & replacing :) Hmmm. Maybe it's time to consider switching to the interface generator.

Revision history for this message
Gábor Csárdi (gabor.csardi) wrote :

Well, the interface generator kinda sucks, so maybe it is not worth switching to that just yet. But this doesn't matter much I guess, if we are going with solution a), the global attribute handling table.

Tamás Nepusz (ntamas)
Changed in igraph:
status: Confirmed → Won't Fix
status: Won't Fix → Fix Committed
assignee: nobody → Tamás Nepusz (ntamas)
Changed in igraph:
status: Fix Committed → Fix Released
Revision history for this message
Gábor Csárdi (gabor.csardi) wrote : Continue on github

The development of igraph has moved to github, so please do not comment on this bug here. You are of course welcome to comment on github, here:
https://github.com/igraph/igraph/issues/117

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.