TagWikiMacros is NOT thread safe

[jun66j5]

TagWikiMacros uses instance variables, self.realms, at tagsplugin/trunk/tractags/macros.py@11934#L160​. However, subclasses of Component create only one instance per Trac environment. Therefore, the component is not thread safe if wsgi.multithread is True (e.g. Apache/win32 with mod_wsgi).

[rjollos]

Interesting. Will this be true anywhere that we make an assignment to an instance variable in a macro? It looks like these two macros may suffer from the same issue:

• codeexamplemacro/trunk/codeexample/code_example_processor.py​
• pollmacro/trunk/tracpoll/tracpoll.py​

[jun66j5]

Replying to rjollos:

Interesting. Will this be true anywhere that we make an assignment to an instance variable in a macro?

If a subclass of Component assigns data from a request to the instance variable, that is not thread safe.
In the TagWikiMacros, it happens at such as rendering [[ListTagged(realms=wiki)]] and [[ListTagged(realms=ticket)]] at the same time in wiki pages.

See ​trac:wiki:TracDev/ComponentArchitecture#Declaringacomponent.

It looks like these two macros may suffer from the same issue:

• codeexamplemacro/trunk/codeexample/code_example_processor.py​

Yes. The above macro has the same issue.

• pollmacro/trunk/tracpoll/tracpoll.py​

It seems that it has the another race condition. Poll should lock the file to dump and load.

[rjollos]

Replying to jun66j5:

Replying to rjollos:
If a subclass of Component assigns data from a request to the instance variable, that is not thread safe.
In the TagWikiMacros, it happens at such as rendering [[ListTagged(realms=wiki)]] and [[ListTagged(realms=ticket)]] at the same time in wiki pages.

If I understand correctly then, the instance variable query will also have this problem. all_realms will not because it's value will always be the same? ... but isn't there a very small chance that one thread could be writing all_realms while another is reading it?

If that were true, maybe we could fix the issue by moving the code to the constructor?:

def __init__(self):
    self.tag_system = TagSystem(self.env)
    self.all_realms = [p.get_taggable_realm()
                       for p in self.tag_system.tag_providers]

It looks like these two macros may suffer from the same issue:

• codeexamplemacro/trunk/codeexample/code_example_processor.py​

Yes. The above macro has the same issue.

Thanks, I will make a ticket and plan to fix it ...

• pollmacro/trunk/tracpoll/tracpoll.py​

It seems that it has the another race condition. Poll should lock the file to dump and load.

... and will do the same for this one (though I think the pickling to file should be replaced with storing to the database anyway, and we have an open ticket for that work item IIRC).

[hasienda]

Replying to rjollos:

Replying to jun66j5:

Replying to rjollos:
If a subclass of Component assigns data from a request to the instance variable, that is not thread safe. ...

If that were true, maybe we could fix the issue by moving the code to the constructor?:
...

Exactly what I've been thinking too.

For currently selected realms we still need a thread-safe alternative to self.realms to propagate the selection between TagWikiMacros methods. I can't think of something else than r/w from/to a req.session attribute, but I haven't looked carefully at the code for a while.

[jun66j5]

Starting point of patch to fix #10636

[jun66j5]

Replying to rjollos:

If I understand correctly then, the instance variable query will also have this problem.

Right.

all_realms will not because it's value will always be the same? ... but isn't there a very small chance that one thread could be writing all_realms while another is reading it?

self.all_realms doesn't have this problem because it is unused.

If that were true, maybe we could fix the issue by moving the code to the constructor?:
...

I don't think it should move to the constructor. TagSystem(self.env).tag_providers dynamically depends on the [components] settings. Also, the moving will not fix this issue....

I created tagsplugin-t10636-r12375.diff​. But the unit tests are missing for the macros. We should add the tests before the patch.

[rjollos]

Replying to jun66j5:

all_realms will not because it's value will always be the same? ... but isn't there a very small chance that one thread could be writing all_realms while another is reading it?

self.all_realms doesn't have this problem because it is unused.

I didn't notice it was unused. It seems like it would have been a problem though if it had been used (code where it is used is commented out).

I don't think it should move to the constructor. TagSystem(self.env).tag_providers dynamically depends on the [components] settings. Also, the moving will not fix this issue....

I had only suggested this for the case in which I thought the instance variable all_realms was used, but I see it is not important now.

Nice usage of partial! ;)

[rjollos]

I've done some work in #10612 to try to utilize the Trac framework when writing tests for wiki formatting. I appreciate if you have a chance to take a look.