TagWikiMacros is NOT thread safe

[Jun Omae]

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).

[Ryan J Ollos]

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​

[Jun Omae]

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.

[Ryan J Ollos]

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).

[Steffen Hoffmann]

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.

[Jun Omae]

Starting point of patch to fix #10636

[Jun Omae]

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.

[Ryan J Ollos]

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! ;)

[Ryan J Ollos]

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.

[Jun Omae]

I reworked at ​https://github.com/jun66j5/trac-tagsplugin/commits/t10636.

• ​443fe05 removed instance variables on TagWikiMacros class and unit tests for TagCloud macro
• ​3785c9e fixed wrong use of _ method

[Steffen Hoffmann]

Replying to jun66j5:

I reworked at ​https://github.com/jun66j5/trac-tagsplugin/commits/t10636.

I've just reviewed and tested your changes. The previous draft looked more spectacular using partial, sure, but this seems not essential. I like the straight-forward clean-up morphing former class variables into method call signature: Well done - as always - and nice catches especially regarding translated macro description (​3785c9e) and more consistent return value in WikiTagProvider.get_all_tags() (​443fe05).

As already commented directly to the changesets, I'd like to see these changes committed here, in order to close the tags-0.7dev development cycle with a final message catalog update.

[Jun Omae]

In 13754:

TagsPlugin: Prevent using instance variables on Component and added unit tests for TagCloud macro (refs #10636)

[Jun Omae]

I've just reviewed and tested your changes.

Thanks for reviewing. I just committed the changes.

Another thing. I've noticed that TagCloud macro doesn't understand query syntax while adding unit tests. It seems that the macro with only realm argument works. If that is intended, I think the documentation should be the following.

tractags/macros.py

@@ -115 +115 @@
     Usage:
 
     {{{
-    [[TagCloud(query,caseless_sort=<bool>,mincount=<n>)]]
+    [[TagCloud(realm=<realm1|realm2|...>,caseless_sort=<bool>,mincount=<n>)]]
     }}}
+    realm::
+      Optional realms to show only tags in the specified realms.
     caseless_sort::
       Whether the tag cloud should be sorted case-sensitive.
     mincount::
       Optional integer threshold to hide tags with smaller count.
-
-    See tags documentation for the query syntax.
     """)
         self.doc_listtagged = N_("""List tagged resources.
 

[Steffen Hoffmann]

In 13757:

TagsPlugin: Salvaging a nice simplification from Jun's early patch, refs #10636.

[Steffen Hoffmann]

Replying to jun66j5:

Another thing. I've noticed that TagCloud macro doesn't understand query syntax while adding unit tests. It seems that the macro with only realm argument works.

This is not intended but a regression coming from [13392].

Further more, finally removing query arguments from get_all_tags() signature in [13427] changed the API in a way, that will make reimplementing the feature less trivial. Looks like we'll have no other choice than to re-introduce some of the very slow code for iterating through tagged resources and corresponding permission checks for constructing custom tag clouds with TagCloud. A macro documentation change might be in order, but a different one, hinting on serious performance pitfalls when using query arguments other than realm.

[Steffen Hoffmann]

In 13791:

TagsPlugin: In web-UI calls provide realms as argument to expand_macro(), refs #10636.

This is a follow-up to [13754].

[Steffen Hoffmann]

In 13793:

TagsPlugin: Re-enable query functionality for TagCloud wiki macro, refs #10636.

[Steffen Hoffmann]

In 13815:

TagsPlugin: Completing preparation for v0.7 release.

Availability of that code as stable, tagged release
closes #2429, #3359, #3610, #3624, #3677, #3754, #3864, #3947, #3983, #4078, #4277, #4503, #4799, #5523, #7787, #7857, #8638, #9057, #9058, #9059, #9060, #9061, #9062, #9063, #9149, #9210, #9521, #9630, #9636, #10032, #10416, #10636, #11096, #11147, #11152, #11274, #11302, #11658 and #11659.

Additionally there are some issues and enhancement requests showing progress,
but known to require more work to resolve them satisfactorily, specifically
refs #2804, #4200, #8747 and #9064.

Thanks to all contributors and followers, that enabled and encouraged a good
portion of this development work.