After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 377539 - (pango-threadsafe) Make Pango thread-safe
(pango-threadsafe)
Make Pango thread-safe
Status: RESOLVED FIXED
Product: pango
Classification: Platform
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: pango-maint
pango-maint
: 499514 586986 (view as bug list)
Depends on: 608695 682846
Blocks: 412678 473862 499514 590788 647778
 
 
Reported: 2006-11-20 21:37 UTC by Behdad Esfahbod
Modified: 2013-03-15 09:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test case for threading (3.95 KB, patch)
2011-03-04 15:29 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
First partial attampt at fixing the thread safety issue. (1.07 KB, patch)
2011-05-16 20:58 UTC, a.pignotti
none Details | Review
static data with locations (4.35 KB, text/plain)
2011-05-17 22:33 UTC, a.pignotti
  Details
test case (1.63 KB, text/x-csrc)
2011-05-17 23:18 UTC, Behdad Esfahbod
  Details
s/static const char*/static const char* const/g (1009 bytes, patch)
2011-05-17 23:39 UTC, a.pignotti
committed Details | Review
Remove static keyword from non actually static data (858 bytes, patch)
2011-05-17 23:39 UTC, a.pignotti
rejected Details | Review
deperecate pango_lookup_aliases (17.73 KB, patch)
2012-08-28 01:56 UTC, Matthias Clasen
committed Details | Review
deal with pango-language.c (2.93 KB, patch)
2012-08-28 02:28 UTC, Matthias Clasen
committed Details | Review
make enum get_type functions thread-safe (5.04 KB, patch)
2012-08-29 02:09 UTC, Matthias Clasen
committed Details | Review
Crashy logo (8.71 KB, image/svg+xml)
2012-09-14 14:25 UTC, Garrett LeSage
  Details
rework config file reading (4.23 KB, patch)
2012-09-15 01:19 UTC, Matthias Clasen
committed Details | Review
make modules.c thread-safe (2.68 KB, patch)
2012-09-15 02:15 UTC, Matthias Clasen
committed Details | Review
deal with fontconfig (20.18 KB, patch)
2012-09-15 04:09 UTC, Matthias Clasen
rejected Details | Review
Poer PangoWin32 to use GWeakRef (2.21 KB, patch)
2013-03-13 09:48 UTC, Fan, Chun-wei
committed Details | Review

Description Behdad Esfahbod 2006-11-20 21:37:41 UTC
Bug for discussion of the upcoming patches to make Pango thread-safe (a bit).
Comment 1 Behdad Esfahbod 2006-11-20 23:51:28 UTC
My thoughts on it:

  http://mail.gnome.org/archives/gtk-i18n-list/2006-November/msg00001.html
Comment 2 Morten Welinder 2006-11-23 02:32:59 UTC
For things like loading of modules, registering types, and other things that
aren't going to happen a lot of times cumulatively, you can probably get away
with a Big Fat Lock.
Comment 3 Behdad Esfahbod 2006-11-23 23:07:46 UTC
(In reply to comment #2)
> For things like loading of modules, registering types, and other things that
> aren't going to happen a lot of times cumulatively, you can probably get away
> with a Big Fat Lock.

Like, a static mutex that we lock/unlock whenever checking if modules are loaded?
Comment 4 Behdad Esfahbod 2011-03-04 14:36:15 UTC
Ok, so we need to types of changes:

  - For all static variables, use g_once_init/leave().

  - For all shared objects (fontmap, ...) use g_object_lock() when that's available.
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2011-03-04 15:29:25 UTC
Created attachment 182488 [details] [review]
test case for threading
Comment 6 Behdad Esfahbod 2011-04-26 22:18:13 UTC
*** Bug 586986 has been marked as a duplicate of this bug. ***
Comment 7 a.pignotti 2011-05-16 20:58:50 UTC
Created attachment 187932 [details] [review]
First partial attampt at fixing the thread safety issue.

I'm trying to tackle this issue. As suggested in comment #4 I'm starting to use g_once_init/leave pairs to protect initialization on static data. In the patch I've only worked on default_font_map to get early feedback on this track. Although this solves the race in initialization code, using the default font map on multi threaded code still look fairly dangerous. I'm especially dubious about pango_cairo_context_set_resolution.
Comment 8 Behdad Esfahbod 2011-05-16 21:33:37 UTC
(In reply to comment #7)
> Created an attachment (id=187932) [details] [review]
> First partial attampt at fixing the thread safety issue.
> 
> I'm trying to tackle this issue.

Thanks!


> As suggested in comment #4 I'm starting to use
> g_once_init/leave pairs to protect initialization on static data. In the patch
> I've only worked on default_font_map to get early feedback on this track.

I believe g_once_init_enter/leave() is designed such that you can call it on the pointer variable direction.  Ie, instead of:

-  if (G_UNLIKELY (!default_font_map))
+  if (g_once_init_enter (&default_font_map_initialized))

you just need:

-  if (G_UNLIKELY (!default_font_map))
+  if (g_once_init_enter (&default_font_map))


> Although this solves the race in initialization code, using the default font
> map on multi threaded code still look fairly dangerous.

We need to add proper locking there, yes.


> I'm especially dubious about pango_cairo_context_set_resolution.

Why?  Context and Layout objects do not need to be threadsafe.
Comment 9 a.pignotti 2011-05-17 12:18:28 UTC
(In reply to comment #8)
 > As suggested in comment #4 I'm starting to use
> > g_once_init/leave pairs to protect initialization on static data. In the patch
> > I've only worked on default_font_map to get early feedback on this track.
> 
> I believe g_once_init_enter/leave() is designed such that you can call it on
> the pointer variable direction.  Ie, instead of:
> 
> -  if (G_UNLIKELY (!default_font_map))
> +  if (g_once_init_enter (&default_font_map_initialized))
> 
> you just need:
> 
> -  if (G_UNLIKELY (!default_font_map))
> +  if (g_once_init_enter (&default_font_map))
I also though about this solution but avoid it as it would require an ugly cast from pointer to gsize and I was not sure about the acceptability of such code. If you think it's ok then we can save a few bytes.

> 
> > I'm especially dubious about pango_cairo_context_set_resolution.
> 
> Why?  Context and Layout objects do not need to be threadsafe.
Sorry, I meant pango_cairo_font_map_set_resolution
Comment 10 Behdad Esfahbod 2011-05-17 18:49:21 UTC
(In reply to comment #9)
> (In reply to comment #8)
>  > As suggested in comment #4 I'm starting to use
> > > g_once_init/leave pairs to protect initialization on static data. In the patch
> > > I've only worked on default_font_map to get early feedback on this track.
> > 
> > I believe g_once_init_enter/leave() is designed such that you can call it on
> > the pointer variable direction.  Ie, instead of:
> > 
> > -  if (G_UNLIKELY (!default_font_map))
> > +  if (g_once_init_enter (&default_font_map_initialized))
> > 
> > you just need:
> > 
> > -  if (G_UNLIKELY (!default_font_map))
> > +  if (g_once_init_enter (&default_font_map))
> I also though about this solution but avoid it as it would require an ugly cast
> from pointer to gsize and I was not sure about the acceptability of such code.
> If you think it's ok then we can save a few bytes.

That's exactly why the function takes gsize, and not int.  Definitely fine with me.


> > > I'm especially dubious about pango_cairo_context_set_resolution.
> > 
> > Why?  Context and Layout objects do not need to be threadsafe.
> Sorry, I meant pango_cairo_font_map_set_resolution

Not worse than the actual font-accessing functions though.

g_object_lock() would make it much easier, but then one has to finish that one.  Maybe adding a mutex to the fontmap is fine afterall...
Comment 11 a.pignotti 2011-05-17 21:38:20 UTC
I've counted around 90 usage of static data, excluding static const data as that should be safe. I think a few of the 90 can be constified. I'll patch the other initializations to be safe. Moreover proper looking seems to be needed around font config calls, I plan to address that after static data. Is this plan ok?
Comment 12 Behdad Esfahbod 2011-05-17 21:54:10 UTC
I don't think we have that many.  For everything in modules.c, a single g_once_init_enter/leave() would do.  And all the parent_class stuff you can ignore since type registration is threadsafe.  I can think of pango-language stuff and default fontmaps, but not much else. What have you got?

Also, ignore pangox and pangoxft.  Even pangoft2 maybe.  For now, in fact, we can focus on pangofc and pangocairo.
Comment 13 a.pignotti 2011-05-17 22:33:03 UTC
Created attachment 188000 [details]
static data with locations

This is a list of the static data that should probably be made thread safe. pango{x,xft,ft2} stuff is at the end.
Comment 14 Colin Walters 2011-05-17 22:44:06 UTC
(In reply to comment #11)

> Is this plan ok?

How are you testing this?  The code from comment 5 ?  Your custom code?
Comment 15 Behdad Esfahbod 2011-05-17 23:18:24 UTC
Created attachment 188004 [details]
test case

I also have a similar test case.  Attached.  Run like:

  ./test-pangocairo-threads 100 100

Though the one in comment 5 ensures that all threads start together, so may hit more bugs more easily.
Comment 16 a.pignotti 2011-05-17 23:38:09 UTC
I'm testing with code in comment 5+valgrind. I'm actually trying to understand the code as thread safety issues are not easy to trigger with tests. I'll test with code in comment 15 too. The first two patches I'm attaching should be anyway safe. The first one make some static data more const so that synchronization is clearly not needed. The other one make some static data that is always overwritten as non-static
Comment 17 a.pignotti 2011-05-17 23:39:06 UTC
Created attachment 188005 [details] [review]
s/static const char*/static const char* const/g
Comment 18 a.pignotti 2011-05-17 23:39:41 UTC
Created attachment 188006 [details] [review]
Remove static keyword from non actually static data
Comment 19 Behdad Esfahbod 2011-05-18 02:14:35 UTC
Review of attachment 188006 [details] [review]:

::: pango/pangowin32-fontmap.c
@@ -1124,3 @@
 charset_name (int charset)
 {
-  static char num[10];

You can't do this.  The function returns this array, so it can't be stack allocated.  Need to restructure code to avoid this.  Take the array as an argument for example.

@@ -1158,3 @@
 ff_name (int ff)
 {
-  static char num[10];

Same.
Comment 20 Behdad Esfahbod 2011-05-18 02:15:46 UTC
Review of attachment 188006 [details] [review]:

In fact, these are only used for debugging, so you can ignore them.
Comment 21 Behdad Esfahbod 2011-05-18 02:16:34 UTC
Review of attachment 188005 [details] [review]:

I'm committing this, but as I'll discuss in another comment, we should remove the alias stuff to pangowin32 and make it g_once_init'able.
Comment 22 a.pignotti 2011-05-18 02:24:02 UTC
(In reply to comment #19)
> Review of attachment 188006 [details] [review]:
> 
> ::: pango/pangowin32-fontmap.c
> @@ -1124,3 @@
>  charset_name (int charset)
>  {
> -  static char num[10];
> 
> You can't do this.  The function returns this array, so it can't be stack
> allocated.  Need to restructure code to avoid this.  Take the array as an
> argument for example.
> 
> @@ -1158,3 @@
>  ff_name (int ff)
>  {
> -  static char num[10];
> 
> Same.
Sorry for this silly mistake. I'll provide a new patch ASAP
Comment 23 Behdad Esfahbod 2011-05-18 02:33:02 UTC
Comment on attachment 188000 [details]
static data with locations

>pango/pango-utils.c:static GHashTable *pango_aliases_ht = NULL;
>pango/pango-utils.c:static GHashTable *config_hash = NULL;

Lets deprecate the alias APIs and move the implementation to pangowin32 that uses it.  Regardless, we need threadsafe hashtables.


>pango/pango-utils.c:static HMODULE pango_dll;

This one's fine.


>pango/pango-utils.c:  static const gchar *result = NULL;
>pango/pango-utils.c:  static const gchar *result = NULL;

g_once_init


>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;
>pango/pango-enum-types.c:  static GType etype = 0;

Don't worry about these.  That's bug 627241.


>pango/pango-ot-ruleset.c:  static GQuark rulesets_quark = 0;

Depends on bug 627240.


>pango/querymodules.c:  static GEnumClass *class = NULL;

Ignore this.  It's not part of the library.


>pango/pangofc-font.c:  static guint engine_type_id = 0;
>pango/pangofc-font.c:  static guint render_type_id = 0;

These are also quarks, so bug 627240 preferably.  Or g_once_init alternatively.


>pango/pangocairo-fcfont.c:  static GEnumClass *class = NULL;
>pango/pangowin32-fontmap.c:static PangoWin32FontMap *default_fontmap = NULL;
>pango/pangowin32-fontmap.c:  static GType object_type = 0;
>pango/pangowin32-fontmap.c:  static GType object_type = 0;
>pango/pango-engine.c:  static PangoEngineShape *fallback_shaper = NULL;
>pango/pangoatsui-fontmap.c:  static GType object_type = 0;
>pango/pango-context.c:  static GQuark cache_quark = 0;
>pango/pango-context.c:  static guint engine_type_id = 0;
>pango/pango-context.c:  static guint render_type_id = 0;
>pango/pango-context.c:  static GEnumClass *class = NULL;
>pango/pangofc-fontmap.c:  static GEnumClass *class = NULL;
>pango/pangofc-fontmap.c:  static gboolean registered_modules = FALSE;
>pango/pangocairo-render.c:static PangoCairoRenderer *cached_renderer = NULL;
>pango/pangocairo-context.c:static PangoCairoContextInfo *
>pango/pangocairo-context.c:  static GQuark context_info_quark;
>pango/pango-fontmap.c:  static GHashTable *warned_fonts = NULL;
>pango/shape.c:      static GQuark warned_quark = 0;

Mostly variations of g_once_init and quark.


>pango/pango-language.c:  static PangoLanguage *result = NULL;
>pango/pango-language.c:  static GHashTable *hash = NULL;
>pango/pango-language.c:  static gboolean initialized = FALSE;
>pango/pango-language.c:  static PangoLanguage * const * languages = NULL;
>pango/pango-language.c:  static GHashTable *hash = NULL;

Big fat lock should do.


>pango/pango-attributes.c:static GHashTable *name_map = NULL;
>pango/pango-attributes.c:  static guint current_type = 0x1000000;

Big fat lock.


>pango/pangocairo-font.c:      static GQuark warned_quark = 0;
>pango/break.c:  static guint engine_type_id = 0;
>pango/break.c:  static guint render_type_id = 0;

Quarks.


>pango/pango-ot-buffer.c:static hb_buffer_t *cached_buffer = NULL;

This one's locked already.


>pango/modules.c:static GList *maps = NULL;
>pango/modules.c:static GSList *registered_engines = NULL;
>pango/modules.c:static GSList *dlloaded_engines = NULL;
>pango/modules.c:static GHashTable *dlloaded_modules;
>pango/modules.c:  static GQuark warned_quark = 0;
>pango/modules.c:  static GEnumClass *class = NULL;
>pango/modules.c:  static gboolean init = FALSE;
>pango/modules.c:      static gboolean no_module_warning = FALSE;

Big fat lock (except for quarks).


>pango/pangocairo-fontmap.c:static PangoFontMap *default_font_map = NULL;

g_once_init.


>pango/pangowin32.c:  static guint engine_type_id = 0;
>pango/pangowin32.c:  static guint render_type_id = 0;

Quarks.


>pango/pangowin32.c:  static guint last = 0; /* Cache of one */
>
>pango/pangox.c:  static GQuark quark = 0;
>pango/pangox.c:  static guint engine_type_id = 0;
>pango/pangox.c:  static guint render_type_id = 0;
>pango/pangox-fontmap.c:static GSList *registered_displays;
>pango/pangox-fontmap.c:static GList *fontmaps = NULL;
>pango/pangox-fontmap.c:  static gboolean registered_modules = FALSE;
>pango/pangox-fontmap.c:static gboolean error_occurred;

Don't touch.  I'll actually remove pangox soon.


>pango/pangoxft-fontmap.c:static GSList *fontmaps = NULL;

g_once_init.

>pango/pangoxft-fontmap.c:static GSList *registered_displays;

Don't bother.

>pango/pangoft2.c:      static char *default_msg = NULL;

Don't bother.

>pango/pangoft2-fontmap.c:static PangoFT2FontMap *pango_ft2_global_fontmap = NULL;

g_once_init.


Thanks for working on this!  So, maybe you can start doing the g_once_init ones, and I'll see if I can get the fixes in glib.  Lets do one commit per one logical change.  Better if you can push a git tree somewhere so I can review and pull.
Comment 24 Behdad Esfahbod 2011-05-18 02:35:30 UTC
Colin, can you look into getting Bug 627240 - add G_DEFINE_QUARK and Bug 627241 - add G_DEFINE_[ENUM|FLAGS]_TYPE in?
Comment 25 a.pignotti 2011-05-18 17:54:43 UTC
I've opened a pango mirror on github:
https://github.com/alexp-sssup/pango

The less-static branch contains an update fix for the static string in pangowin32-fontmap.c

The safe-static branch contains the first patch (detailed below) to make static data thread safe. New patches will be added in that branch


(In reply to comment #23)
> >pango/pango-utils.c:static GHashTable *pango_aliases_ht = NULL;
> >pango/pango-utils.c:static GHashTable *config_hash = NULL;
> 
> Lets deprecate the alias APIs and move the implementation to pangowin32 that
> uses it.  Regardless, we need threadsafe hashtables.
Should I take a look at making hashtables thread safe myself?
> 
> 
> >pango/pango-utils.c:static HMODULE pango_dll;
> 
> This one's fine.
You mean we can leave it alone?
> 
> 
> >pango/pango-utils.c:  static const gchar *result = NULL;
> >pango/pango-utils.c:  static const gchar *result = NULL;
> 
> g_once_init
Those are fixed in the safe-static branch
Comment 26 Behdad Esfahbod 2011-05-18 18:53:50 UTC
(In reply to comment #25)

> (In reply to comment #23)
> > >pango/pango-utils.c:static GHashTable *pango_aliases_ht = NULL;
> > >pango/pango-utils.c:static GHashTable *config_hash = NULL;
> > 
> > Lets deprecate the alias APIs and move the implementation to pangowin32 that
> > uses it.  Regardless, we need threadsafe hashtables.
>
> Should I take a look at making hashtables thread safe myself?

I talked to Ryan Lortie, and also filed Bug 650459 - hash table consistency while calling destroy notify funcs.  With that fixed, we can just wrap all our hashtable ops with a recursive-mutex and should be fine.  When I said we can't just wrap it with a lock I wasn't thinking about recursive-mutexes.


> > >pango/pango-utils.c:static HMODULE pango_dll;
> > 
> > This one's fine.
> You mean we can leave it alone?

Yes.


> > >pango/pango-utils.c:  static const gchar *result = NULL;
> > >pango/pango-utils.c:  static const gchar *result = NULL;
> > 
> > g_once_init
> Those are fixed in the safe-static branch

Thanks.  Will check.
Comment 27 a.pignotti 2011-05-19 18:53:42 UTC
> 
> >pango/pangocairo-fcfont.c:  static GEnumClass *class = NULL;
> >pango/pangowin32-fontmap.c:static PangoWin32FontMap *default_fontmap = NULL;
> >pango/pangowin32-fontmap.c:  static GType object_type = 0;
> >pango/pangowin32-fontmap.c:  static GType object_type = 0;
> >pango/pango-engine.c:  static PangoEngineShape *fallback_shaper = NULL;
> >pango/pangoatsui-fontmap.c:  static GType object_type = 0;
> >pango/pango-context.c:  static GQuark cache_quark = 0;
> >pango/pango-context.c:  static guint engine_type_id = 0;
> >pango/pango-context.c:  static guint render_type_id = 0;
> >pango/pango-context.c:  static GEnumClass *class = NULL;
> >pango/pangofc-fontmap.c:  static GEnumClass *class = NULL;
> >pango/pangofc-fontmap.c:  static gboolean registered_modules = FALSE;
> >pango/pangocairo-render.c:static PangoCairoRenderer *cached_renderer = NULL;
> >pango/pangocairo-context.c:static PangoCairoContextInfo *
> >pango/pangocairo-context.c:  static GQuark context_info_quark;
> >pango/pango-fontmap.c:  static GHashTable *warned_fonts = NULL;
> >pango/shape.c:      static GQuark warned_quark = 0;
> 
> Mostly variations of g_once_init and quark.
I'm waiting news for the quark issue before tackling those
> 
> 
> >pango/pango-language.c:  static PangoLanguage *result = NULL;
> >pango/pango-language.c:  static GHashTable *hash = NULL;
> >pango/pango-language.c:  static gboolean initialized = FALSE;
> >pango/pango-language.c:  static PangoLanguage * const * languages = NULL;
> >pango/pango-language.c:  static GHashTable *hash = NULL;
> 
> Big fat lock should do.
commit 23f7d0f43fb86105e1526cd7016149c179c20b89 solves those. I've used a g_once_enter/leave pair and two mutexes
Comment 28 a.pignotti 2011-05-20 21:11:18 UTC
>>pango/pango-attributes.c:static GHashTable *name_map = NULL;
>>pango/pango-attributes.c:  static guint current_type = 0x1000000;
>
>Big fat lock.
>
>
>>pango/modules.c:static GList *maps = NULL;
>>pango/modules.c:static GSList *registered_engines = NULL;
>>pango/modules.c:static GSList *dlloaded_engines = NULL;
>>pango/modules.c:static GHashTable *dlloaded_modules;
>>pango/modules.c:  static GQuark warned_quark = 0;
>>pango/modules.c:  static GEnumClass *class = NULL;
>>pango/modules.c:  static gboolean init = FALSE;
>>pango/modules.c:      static gboolean no_module_warning = FALSE;
>
>Big fat lock (except for quarks).
>
>
>>pango/pangocairo-fontmap.c:static PangoFontMap *default_font_map = NULL;
>
>g_once_init.

Those should be also fixed in the github branch.

Are the patches ok until now? Should I work to make the quarks safe or wait?
Comment 29 a.pignotti 2011-05-22 16:22:40 UTC
The patch on modules.c caused a deadlock. This should be solved in the updated commit.
Comment 30 a.pignotti 2011-05-22 17:34:20 UTC
In modules.c:295 the g_type_module_use function is used. As it is not thread safe the module may end up being loaded multiple times. Any suggestion on how to handle this?
Comment 31 Behdad Esfahbod 2011-05-22 17:47:16 UTC
Humm?  We should have a big fat mutex for modules.c.  Why wouldn't that do?
Comment 32 a.pignotti 2011-05-22 17:59:16 UTC
Yes, locking around the call should be ok. I'll go down that road
Comment 33 a.pignotti 2011-05-25 13:51:25 UTC
>>pango/pangoxft-fontmap.c:static GSList *fontmaps = NULL;
>
>g_once_init.

>>pango/pangoft2-fontmap.c:static PangoFT2FontMap *pango_ft2_global_fontmap = NULL;
>
>g_once_init.

For both it seems more appropriate to use a mutex, as those are modified around the code. Don't you think?
Comment 34 a.pignotti 2011-05-25 14:16:49 UTC
>pango/pangocairo-fcfont.c:  static GEnumClass *class = NULL;
>pango/pangowin32-fontmap.c:  static GType object_type = 0;
>pango/pangowin32-fontmap.c:  static GType object_type = 0;
>pango/pango-engine.c:  static PangoEngineShape *fallback_shaper = NULL;
>pango/pangoatsui-fontmap.c:  static GType object_type = 0;
>pango/pango-context.c:  static GEnumClass *class = NULL;
>pango/pangofc-fontmap.c:  static GEnumClass *class = NULL;
>pango/pangofc-fontmap.c:  static gboolean registered_modules = FALSE;
Fixed those using g_once_enter/leave

>pango/pangowin32-fontmap.c:static PangoWin32FontMap *default_fontmap = NULL;
It seems that also this one must be handled with a mutex, the variable is also used in pango_win32_shutdown_display


>pango/pango-context.c:  static GQuark cache_quark = 0;
>pango/pango-context.c:  static guint engine_type_id = 0;
>pango/pango-context.c:  static guint render_type_id = 0;
>pango/pangocairo-context.c:  static GQuark context_info_quark;
>pango/shape.c:      static GQuark warned_quark = 0;
Waiting news about handling quarks

>pango/pangocairo-render.c:static PangoCairoRenderer *cached_renderer = NULL;
This seems to be already locked

>pango/pango-fontmap.c:  static GHashTable *warned_fonts = NULL;
This will also need a mutex

Patches for the fixed issues are available from github.

I think all the things fixable using g_once_init are done. It would be really nice to have a review of the current status and a suggestion about what to do next.
Comment 35 Behdad Esfahbod 2011-05-25 14:39:11 UTC
(In reply to comment #33)
> >>pango/pangoxft-fontmap.c:static GSList *fontmaps = NULL;
> >
> >g_once_init.

You're right about this one.


> >>pango/pangoft2-fontmap.c:static PangoFT2FontMap *pango_ft2_global_fontmap = NULL;
> >
> >g_once_init.

This one we initialize once only, right?


> For both it seems more appropriate to use a mutex, as those are modified around
> the code. Don't you think?
Comment 36 Behdad Esfahbod 2011-05-25 14:42:20 UTC
(In reply to comment #34)
> >pango/pangocairo-fcfont.c:  static GEnumClass *class = NULL;
> >pango/pangowin32-fontmap.c:  static GType object_type = 0;
> >pango/pangowin32-fontmap.c:  static GType object_type = 0;
> >pango/pango-engine.c:  static PangoEngineShape *fallback_shaper = NULL;
> >pango/pangoatsui-fontmap.c:  static GType object_type = 0;
> >pango/pango-context.c:  static GEnumClass *class = NULL;
> >pango/pangofc-fontmap.c:  static GEnumClass *class = NULL;
> >pango/pangofc-fontmap.c:  static gboolean registered_modules = FALSE;
> Fixed those using g_once_enter/leave
> 
> >pango/pangowin32-fontmap.c:static PangoWin32FontMap *default_fontmap = NULL;
> It seems that also this one must be handled with a mutex, the variable is also
> used in pango_win32_shutdown_display

One can argue that if you call shutdown_display() (or similar functions in other backends), it's your responsibility not to use that display/backend anymore.  So I wouldn't worry about this one too much.


> >pango/pango-context.c:  static GQuark cache_quark = 0;
> >pango/pango-context.c:  static guint engine_type_id = 0;
> >pango/pango-context.c:  static guint render_type_id = 0;
> >pango/pangocairo-context.c:  static GQuark context_info_quark;
> >pango/shape.c:      static GQuark warned_quark = 0;
> Waiting news about handling quarks

You can go ahead and define a PANGO_DEFINE_QUARK(name) for now, that would define a function called name##_get_quark(), and use that around the code.


> >pango/pangocairo-render.c:static PangoCairoRenderer *cached_renderer = NULL;
> This seems to be already locked
> 
> >pango/pango-fontmap.c:  static GHashTable *warned_fonts = NULL;
> This will also need a mutex
> 
> Patches for the fixed issues are available from github.

Thanks!


> I think all the things fixable using g_once_init are done. It would be really
> nice to have a review of the current status and a suggestion about what to do
> next.

I'll try to do that this week.
Comment 37 a.pignotti 2011-05-25 14:59:48 UTC
> > >>pango/pangoft2-fontmap.c:static PangoFT2FontMap *pango_ft2_global_fontmap = NULL;
> > >
> > >g_once_init.
> 
> This one we initialize once only, right?
> 

> > >pango/pangowin32-fontmap.c:static PangoWin32FontMap *default_fontmap = NULL;
> > It seems that also this one must be handled with a mutex, the variable is also
> > used in pango_win32_shutdown_display
>
> One can argue that if you call shutdown_display() (or similar functions in
> other backends), it's your responsibility not to use that display/backend
> anymore.  So I wouldn't worry about this one too much.

Both are now fixed.
Comment 38 a.pignotti 2011-05-25 15:19:30 UTC
>> >>pango/pangoxft-fontmap.c:static GSList *fontmaps = NULL;
>> >
>> >g_once_init.
>
>You're right about this one.

Fixed this one as well on github
Comment 39 a.pignotti 2011-05-27 17:30:07 UTC
> > >pango/pango-context.c:  static GQuark cache_quark = 0;
> > >pango/pango-context.c:  static guint engine_type_id = 0;
> > >pango/pango-context.c:  static guint render_type_id = 0;
> > >pango/pangocairo-context.c:  static GQuark context_info_quark;
> > >pango/shape.c:      static GQuark warned_quark = 0;
> > Waiting news about handling quarks
> 
> You can go ahead and define a PANGO_DEFINE_QUARK(name) for now, that would
> define a function called name##_get_quark(), and use that around the code.
> 
Could you please elaborate on this, I'm not sure about what you mean. How would such macro make the code thread safe?
Comment 40 Behdad Esfahbod 2011-05-27 17:40:49 UTC
(In reply to comment #39)
> > > >pango/pango-context.c:  static GQuark cache_quark = 0;
> > > >pango/pango-context.c:  static guint engine_type_id = 0;
> > > >pango/pango-context.c:  static guint render_type_id = 0;
> > > >pango/pangocairo-context.c:  static GQuark context_info_quark;
> > > >pango/shape.c:      static GQuark warned_quark = 0;
> > > Waiting news about handling quarks
> > 
> > You can go ahead and define a PANGO_DEFINE_QUARK(name) for now, that would
> > define a function called name##_get_quark(), and use that around the code.
> > 
> Could you please elaborate on this, I'm not sure about what you mean. How would
> such macro make the code thread safe?

Try something like this:

#define PANGO_DEFINE_QUARK(name, str) \
GQuark \
name##_get_quark (void) \
{ \
  static GQuark quark = 0; \
  if (G_UNLIKELY (!quark) \
    quark = g_quark_from_static_string (str); \
  return quark; \
}

I was going to use g_once_init_enter/leave but I noticed that since g_quark_from_static_string is safe and idempotent, and int operations are atomic, this code is threadsafe.  No need for more involved stuff.

Should go in pango-impl-utils.c for now.

Then you can use it to define all your quarks.

Or, given that no magic is needed, maybe just leave the quark ones as is?
Comment 41 a.pignotti 2011-05-29 00:27:35 UTC
Leaving them as they are seems ok to me.

I think the next step could be protecting FontConfig calls as those seem not safe
Comment 42 a.pignotti 2011-06-02 00:23:50 UTC
I've pushed on github two patches that deals with fontconfig

The first one only protect with a mutex the ones using the global FcConfig, as those are at first sight the only unsafe calls.

As I still had crashes on the testthreads test I also protected all the other fontconfig calls. I'm not sure if that is right though.

I still think the modules code needs to be made safe, moreover I hit some crashes that I think may be related to the safety of hash maps.

I tried to make the basic module thread safe. The patch is the last on on github.

With all the patches applied I managed to get the testthread example running and it crashes only once in a while. It would be very nice at this point to have a bit of review and guidance. Moreover, I was wondering if the patches, at least the simpler ones dealing with initialization, could be accepted upstream. Please consider that until now multithreaded code was completely unsupported and those should have no effect on single threaded apps.
Comment 43 Behdad Esfahbod 2011-06-02 17:27:01 UTC
(In reply to comment #42)
> I've pushed on github two patches that deals with fontconfig
> 
> The first one only protect with a mutex the ones using the global FcConfig, as
> those are at first sight the only unsafe calls.
> 
> As I still had crashes on the testthreads test I also protected all the other
> fontconfig calls. I'm not sure if that is right though.

Will take a look.


> I still think the modules code needs to be made safe, moreover I hit some
> crashes that I think may be related to the safety of hash maps.
> 
> I tried to make the basic module thread safe. The patch is the last on on
> github.
> 
> With all the patches applied I managed to get the testthread example running
> and it crashes only once in a while. It would be very nice at this point to
> have a bit of review and guidance. Moreover, I was wondering if the patches, at
> least the simpler ones dealing with initialization, could be accepted upstream.
> Please consider that until now multithreaded code was completely unsupported
> and those should have no effect on single threaded apps.

I don't think the modules need any change.  I started reviewing your changes, most look trivial and good to go. The modules.c changes however looked wrong and needed detailed review, so I postponed it.  With correct modules.c locking, you don't have to worry about the shaper modules I guess.  Will start merging the trivial parts.
Comment 44 a.pignotti 2011-06-18 17:24:37 UTC
Any news?
Comment 45 Behdad Esfahbod 2011-06-20 16:39:14 UTC
Didn't get to do yet.  Will try tonight.
Comment 46 Jasper St. Pierre (not reading bugmail) 2011-08-02 23:15:14 UTC
Can I get on a ping on this too? This would be helpful for the Shell so we can load SVG off of the main thread.
Comment 47 Behdad Esfahbod 2011-08-03 20:43:25 UTC
Many of the commits are trivial and can go in.  But the nontrivial ones need more work, and so far I haven't got the time to review and fix them :(.
Comment 48 Matthias Clasen 2012-08-22 02:38:47 UTC
*** Bug 499514 has been marked as a duplicate of this bug. ***
Comment 49 Matthias Clasen 2012-08-28 01:55:32 UTC
(In reply to comment #23)
> (From update of attachment 188000 [details])
> >pango/pango-utils.c:static GHashTable *pango_aliases_ht = NULL;
> >pango/pango-utils.c:static GHashTable *config_hash = NULL;
> 
> Lets deprecate the alias APIs and move the implementation to pangowin32 that
> uses it.  Regardless, we need threadsafe hashtables.
> 

Here's a patch that does this part.
Comment 50 Matthias Clasen 2012-08-28 01:56:21 UTC
Created attachment 222604 [details] [review]
deperecate pango_lookup_aliases
Comment 51 Behdad Esfahbod 2012-08-28 02:17:13 UTC
Ack.  I'm starting reviewing the patchset.
Comment 52 Matthias Clasen 2012-08-28 02:28:05 UTC
Created attachment 222606 [details] [review]
deal with pango-language.c
Comment 53 Behdad Esfahbod 2012-08-28 04:33:46 UTC
I pushed out the trivial bits.  Trickier parts remain.
Comment 54 Matthias Clasen 2012-08-29 02:09:02 UTC
Created attachment 222698 [details] [review]
make enum get_type functions thread-safe
Comment 55 Behdad Esfahbod 2012-08-29 02:15:59 UTC
To summarize, what's left to do:

- Switch to G_DEFINE_QUARK.  Small potatoes.

- Figure out config reading stuff.  Easy.

- Figure out module loading stuff.  Medium.

- Figure out Fontconfig.  Perhaps switch to using per PangoFcFontmap FcConfig's.  Medium.

- Figure out g_object_set_qdata-based thread-safety.  Medium, the API for that is being developed.

- Figure out general thread-safety, or at a minimum, PangoFc level.  Hard.  Needs careful examination of all objects involved (PangoFcFontmap, PantoFcFontset, PangoFcFont, ...).

I probably will attack them in that order approximately.  The good news is, we get more threadsafe as we go, so it's not a black/white decision.
Comment 56 Matthias Clasen 2012-08-29 02:21:23 UTC
Review of attachment 222698 [details] [review]:

.
Comment 57 Garrett LeSage 2012-09-14 14:25:10 UTC
Created attachment 224324 [details]
Crashy logo

Inkscape sometimes produces SVGs that are instant crashers for Nautilus. As soon as it tries to render one of these SVGs, it crashes immediately.

I was chatting in #gnome-design about this, and it was suggested that I upload the offending SVG to this bug.

I'm on Fedora 17 with GNOME 3.4, and Nautilus immediately crashes whenever it attempts to render this graphic, regardless of file name or directory it is contained within.

Note: The file selector's thumbnail preview works properly, oddly enough.
Comment 58 Alexander Larsson 2012-09-14 14:33:25 UTC
Garrett: It doesn't crash here, but it does use text so will render with pango, which may race with svg rendering on the main thread when nautilus renders the thumbnail in a thread.
Comment 59 Matthias Clasen 2012-09-15 01:19:01 UTC
Created attachment 224375 [details] [review]
rework config file reading
Comment 60 Matthias Clasen 2012-09-15 02:15:02 UTC
Created attachment 224379 [details] [review]
make modules.c thread-safe
Comment 61 Matthias Clasen 2012-09-15 04:09:20 UTC
Created attachment 224381 [details] [review]
deal with fontconfig
Comment 62 Matthias Clasen 2012-09-15 04:19:19 UTC
After these patches, I see failures stemming from cross-thread use of fontmaps
Comment 63 Behdad Esfahbod 2012-09-16 04:15:10 UTC
(In reply to comment #59)
> Created an attachment (id=224375) [details] [review]
> rework config file reading

Humm.  For some reason this patch doesn't seem to apply.  Can you please check?  Did you work off something other than master perhaps?

And, feel free to push BTW.
Comment 64 Behdad Esfahbod 2012-09-16 04:18:24 UTC
Review of attachment 224379 [details] [review]:

Same thing here.  Doesn't apply for me.  Not sure if I'm doing something ridiculously wrong...

In the future we can probably make the common case lock-free, but I'm more than happy to take this now.  Feel free to just push.
Comment 65 Behdad Esfahbod 2012-09-16 04:21:29 UTC
Review of attachment 224381 [details] [review]:

This is definitely a good start, but I a bit excessive I think.  We have to clearly defined what the requirements are and filter the patch to adhere to those.  For example, looks to me like the patch as is would do multiple lockings where the FcPattern being used is either still being constructed and hence not shared, or is immutable.

Also, I really need to figure out whether I want to go down this path or just fix Fontconfig instead.
Comment 66 Behdad Esfahbod 2012-09-16 04:23:13 UTC
Thanks again Matthias.  After we resolve your two first patches, I can kick this for a couple hours (Sunday night) and hopefully we'll have something working.  The two major items after those patches of yours are indeed 1) fontconfig, and 2) fontmap.  The first one at least we have your patch as fallback, for the second one, it doesn't sound like an awful lot of work to me, but rather tricky.  I'll see what I get to do with them.
Comment 67 Matthias Clasen 2012-09-16 15:06:48 UTC
Oh, they didn't apply because I had forgotten to attach one preliminary config parsing patch. Sorry about that.

I've pushed the config parsing and modules.c changes now.
Comment 68 Matthias Clasen 2012-09-16 15:10:37 UTC
On the fontconfig locking, I really wasn't able to discern if there's any part of the fontconfig api that would be safe to use without locking. 

I started reading FcPatternGetDouble, and that quickly lead me to

FcObjectFromName -> FcObjectFindByName -> FcObjectInsert

which is modifying a global hash table...
Comment 69 Behdad Esfahbod 2012-10-10 19:50:45 UTC
Fontconfig is fixed now.  It's in wip/threadsafety branch, but I expect it to be merged in the next couple of days.  Now I see crashes in pangofc's hash tables, which is expected and the last remaining piece of the puzzle.  Will get to them.
Comment 70 Behdad Esfahbod 2012-11-15 08:19:14 UTC
To update here: I created a branch of about twenty commits, trying to make PangoFcFontMap threadsafe.  I failed.  After much talk with desrt, I now think we want to declare PangoFontMap NOT threadsafe, and instad make pango_cairo_font_map_get_default() return a thread-private copy.  I think that can be made to work quickly, but I have not got to do that.
Comment 71 Behdad Esfahbod 2013-01-03 09:54:17 UTC
Ok, good news!

Fontconfig is threadsafe in master now.

Pango is threadsafe now!  See log below.

What remains to do:

  - Wait for fontconfig release and require it,

  - Enable the test-pangocairo-threads by default, and increase number of threads and iterations.


commit dd90b07699fe5bf0ea5dfee7e907662e8ca8280b
Author: Behdad Esfahbod <behdad@behdad.org>
Date:   Thu Jan 3 03:52:09 2013 -0600

    Make pango threadsafe!
    
    By way of declaring fontmaps NOT threadsafe, and making
    pango_cairo_font_map_get_default() return a thread-private fontmap.
    
    test-pangocairo-threads doesn't crash anymore, if used with fontconfig
    master.

commit 00997204f7df5f4e2f75f8c1c2f72130f302e7c6
Author: Behdad Esfahbod <behdad@behdad.org>
Date:   Thu Jan 3 03:51:10 2013 -0600

    Add test-pangocairo-threads.c
    
    Currently is not run automatically.  But run it with args "10 100" and
    see it crash...

commit 97b14f5ca62d37311798ecdd39ea4368785a2fa8
Author: Behdad Esfahbod <behdad@behdad.org>
Date:   Wed Nov 7 10:58:40 2012 -0800

    Link to gthread-2.0
Comment 72 Behdad Esfahbod 2013-01-03 10:05:44 UTC
And update docs.
Comment 73 Fan, Chun-wei 2013-03-13 09:48:42 UTC
Created attachment 238766 [details] [review]
Poer PangoWin32 to use GWeakRef

Hi,

Sorry if I posted in the wrong bug report...

I did a little bit of work to port the PangoWin32 code (specifically pangowin32.c and pangowin32-fontmap.c) to use the GWeakRef api, and it does seem to me that it fares better in the test-pangocairo-threads test program (after I changed the snprintf call in it to g_snprintf), as I did manage to have it run without crashing in the process.

However, what I get there is a bunch of:

(test-pangocairo-threads.exe:4340): Pango-WARNING **: All font fallbacks failed!
!!!

and the produced .png is different from the reference .png.  Yet to investigate why.

I guess this might be a head start for thread safety on PangoWin32, so this is what I have now.

By the way, I was looking at Behdad's bug report on making pangowin32-fontmap use G_DEFINE_TYPE-can someone elaborate what work needs to be done there?  Hopefully I can look into that as well ASAP.

With blessings, thank you!
Comment 74 Behdad Esfahbod 2013-03-15 08:33:12 UTC
(In reply to comment #73)
> Created an attachment (id=238766) [details] [review]
> Poer PangoWin32 to use GWeakRef
> 
> Hi,
> 
> Sorry if I posted in the wrong bug report...
> 
> I did a little bit of work to port the PangoWin32 code (specifically
> pangowin32.c and pangowin32-fontmap.c) to use the GWeakRef api, and it does
> seem to me that it fares better in the test-pangocairo-threads test program
> (after I changed the snprintf call in it to g_snprintf), as I did manage to
> have it run without crashing in the process.

Great!  Please push the g_snprintf() change out.


> However, what I get there is a bunch of:
> 
> (test-pangocairo-threads.exe:4340): Pango-WARNING **: All font fallbacks
> failed!
> !!!

Did your patch from the other bug fix that?


> and the produced .png is different from the reference .png.  Yet to investigate
> why.

Keep us posted.  Please file new bugs.


> I guess this might be a head start for thread safety on PangoWin32, so this is
> what I have now.

That's great.  Thanks!

> By the way, I was looking at Behdad's bug report on making pangowin32-fontmap
> use G_DEFINE_TYPE-can someone elaborate what work needs to be done there? 
> Hopefully I can look into that as well ASAP.
> 
> With blessings, thank you!
Comment 75 Behdad Esfahbod 2013-03-15 08:37:35 UTC
Marking this fixed.  Please file new bugs for remaining issues.
Comment 76 Behdad Esfahbod 2013-03-15 09:58:56 UTC
Comment on attachment 238766 [details] [review]
Poer PangoWin32 to use GWeakRef

I committed these.