Opened 16 months ago

# TagWikiMacros is NOT thread safe

Reported by: Owned by: jun66j5 hasienda normal TagsPlugin normal threadsafe 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).

### comment:1 follow-up: ↓ 2 Changed 16 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: ↓ 3 Changed 16 months ago by jun66j5

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.

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: ↓ 4 ↓ 5 Changed 16 months ago by 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 16 months ago by hasienda

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 16 months ago by jun66j5

Starting point of patch to fix #10636

### comment:5 in reply to: ↑ 3 ; follow-up: ↓ 6 Changed 16 months ago by jun66j5

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 16 months ago by rjollos

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

### Modify Ticket

Change Properties