search

groue / GRMustache

Flexible and production-ready Mustache templates for MacOS Cocoa and iOS
http://mustache.github.com/
MIT License
1.44k stars 190 forks source link

Thread safety #65

Closed ali-rantakari closed 10 years ago

ali-rantakari commented 10 years ago

I would like to be able to parallelize the rendering of templates, but GRMustache doesn't seem to be thread safe for this. Most notably I see that static CFMutableDictionaries are used for caching data in a few places, without synchronization.

It would be great if thread safety was guaranteed for template rendering.

Thank you for this very useful library!

groue commented 10 years ago

Hi Ali.

Let's first address the issue. I'll cope with performance later.

Commit e74301f removes all the static dictionaries I've seen - they were all used as caches. I haven't carefully checked the rest of the code yet, though.

groue commented 10 years ago

Thanks for posting this issue, BTW

groue commented 10 years ago

Do you think of any other kind of other thread-unsafe pattern I could look after?

ali-rantakari commented 10 years ago

Those global caches were the only thread safety issue that my cursory examination of the source yielded — thanks for the dramatically quick fix! I will get back to this if I run into any crashes (and manage to get a stack trace.)

groue commented 10 years ago

You're welcome :-)

I've looked at the rest of the code. Please take care of those three mutable & unsafe classes in GRMustache:

  1. [GRMustacheConfiguration defaultConfiguration]
  2. One should use a GRMustacheTemplateRepository instance on a single thread.
  3. One should use a instance of a mutable GRMustacheContext subclass on a single thread.

One does not commonly see those objects, though: there is little chance your code triggers threading issues in GRMustache.

The last two points are already well handled by high-level methods of the library, such as +[GRMustacheTemplate templateFrom...] & -[GRMustacheTemplate render...]. The first point deserves more thought & documentation. If you touch [GRMustacheConfiguration defaultConfiguration], consider doing it early, before any rendering. Or make one copy per thread and assign it to a GRMustacheTemplateRepository instance.

Tell me if you need more information until I fix the doc.

groue commented 10 years ago

Last two commits make GRMustacheTemplateRepository safer: you can now load templates from a repository on various threads.

groue commented 10 years ago

GRMustache v6.8.4 guarantees thread-safety of non-mutating methods.