Modify

Opened 22 months ago

Closed 6 months ago

#10636 closed defect (fixed)

TagWikiMacros is NOT thread safe

Reported by: jun66j5 Owned by: hasienda
Priority: normal Component: TagsPlugin
Severity: normal Keywords: threadsafe
Cc: Trac Release: 0.12

Description

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

Attachments (1)

tagsplugin-t10636-r12375.diff (7.2 KB) - added by jun66j5 22 months ago.
Starting point of patch to fix #10636

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 22 months ago by 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:

comment:2 in reply to: ↑ 1 ; follow-up: Changed 22 months ago by 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:

Yes. The above macro has the same issue.

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

comment:3 in reply to: ↑ 2 ; follow-ups: Changed 22 months ago by 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:

Yes. The above macro has the same issue.

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

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

comment:4 in reply to: ↑ 3 Changed 22 months ago by 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.

Changed 22 months ago by jun66j5

Starting point of patch to fix #10636

comment:5 in reply to: ↑ 3 ; follow-up: Changed 22 months ago by 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.

comment:6 in reply to: ↑ 5 Changed 22 months ago by 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! ;)

comment:7 Changed 22 months ago by 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.

comment:8 follow-up: Changed 6 months ago by jun66j5

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

comment:9 in reply to: ↑ 8 ; follow-up: Changed 6 months ago by hasienda

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.

comment:10 Changed 6 months ago by jun66j5

In 13754:

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

comment:11 in reply to: ↑ 9 ; follow-up: Changed 6 months ago by jun66j5

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

    diff --git a/tractags/macros.py b/tractags/macros.py
    index 811f8a9..49eaeec 100644
    a b class TagWikiMacros(TagTemplateProvider): 
    115115    Usage: 
    116116 
    117117    {{{ 
    118     [[TagCloud(query,caseless_sort=<bool>,mincount=<n>)]] 
     118    [[TagCloud(realm=<realm1|realm2|...>,caseless_sort=<bool>,mincount=<n>)]] 
    119119    }}} 
     120    realm:: 
     121      Optional realms to show only tags in the specified realms. 
    120122    caseless_sort:: 
    121123      Whether the tag cloud should be sorted case-sensitive. 
    122124    mincount:: 
    123125      Optional integer threshold to hide tags with smaller count. 
    124  
    125     See tags documentation for the query syntax. 
    126126    """) 
    127127        self.doc_listtagged = N_("""List tagged resources. 
    128128 

comment:12 Changed 6 months ago by hasienda

In 13757:

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

comment:13 in reply to: ↑ 11 Changed 6 months ago by hasienda

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.

comment:14 Changed 6 months ago by hasienda

In 13791:

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

This is a follow-up to [13754].

comment:15 Changed 6 months ago by hasienda

In 13793:

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

comment:16 Changed 6 months ago by hasienda

  • Resolution set to fixed
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.