LOCALE_IO isn't threadsafe, used around threads...

Bug #1674107 reported by Chris Pavlina
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
KiCad
In Progress
Low
Chris Pavlina

Bug Description

LOCALE_IO calls setlocale(), which is explicitly listed as non-threadsafe in the Linux man pages:

       ┌────────────┬───────────────┬────────────────────────────┐
       │Interface │ Attribute │ Value │
       ├────────────┼───────────────┼────────────────────────────┤
       │setlocale() │ Thread safety │ MT-Unsafe const:locale env │
       └────────────┴───────────────┴────────────────────────────┘

(edit: damn you launchpad for reformatting that!)

Functions marked "const:locale" modify the locale (duh) and thus should not be used in the presence of functions marked "locale" (which read the locale) in other threads. In theory, the following should be safe:

1. Create LOCALE_IO (MT-Unsafe const:locale)
2. Start threads (MT-Safe locale)
3. Join threads
4. Destroy LOCALE_IO (MT-Unsafe const:locale)

Now... we do this "correctly", usually. For instance, the footprint loader creates a LOCALE_IO before spawning the worker threads, lets the workers operator, then joins them before the LOCALE_IO is destroyed.

HOWEVER. This causes a problem, because now whatever is following this process MUST completely lock up KiCad. Otherwise, a user could go to another part of KiCad while it works, and start *other* threaded procedures that are "MT-Unsafe const:locale" while the "MT-Safe locale" part is running.

So, it works currently, but it's very fragile.

Now...I'm the one who's trying to make footprint loading asynchronous, so I guess this is on me to fix.

uselocale() exists on Linux and macOS, and allows configuration of a per-thread locale. On MSYS/mingw, the method is instead to use the Windows API function _configthreadlocale(_ENABLE_PER_THREAD_LOCALE), which makes setlocale() threadsafe and per-thread within the thread that called it.

My plan for fixing this is to rewrite LOCALE_IO to use uselocale or _configthreadlocale so that it sets the locale only for the thread creating it, then move all uses of LOCALE_IO into a single-thread context (i.e. all worker threads set their own locale, rather than the function that starts them doing it globally first).

description: updated
Changed in kicad:
status: Triaged → In Progress
Revision history for this message
Chris Pavlina (pavlina-chris) wrote :

Change of plans. On mingw, _configthreadlocale() is --- drumroll please! --- a no-op.

Currently, LOCALE_IO is only used in a threaded environment in one place. Instead of making LOCALE_IO per-thread and threadsafe, I'm going to make it *fully* single threaded. I can refactor the footprint loader not to require it within the loading threads.

Since this is only really applicable when the footprint loader is asynchronous, I'm going to do it on top of my existing footprint loader async refactor. Hopefully I'll have a patchset for that soon.

Revision history for this message
Chris Pavlina (pavlina-chris) wrote :

Downgrading to low because the current code *is* threadsafe, just fragile. It's only a major bug in my async branch - where I've already fixed it asynchronous prefetch and synchronous parse.

Changed in kicad:
importance: Medium → Low
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.