Ticket UUID: | c6057948e5f884435ee35517008db0205f286a5e | |||
Title: | tsv commands break thread access rules for Tcl values | |||
Type: | Bug | Version: | ||
Submitter: | dgp | Created on: | 2018-03-29 23:02:20 | |
Subsystem: | 80. Thread Package | Assigned To: | aku | |
Priority: | 5 Medium | Severity: | Critical | |
Status: | Open | Last Modified: | 2018-04-03 12:45:07 | |
Resolution: | None | Closed By: | nobody | |
Closed on: | ||||
Description: |
Multi-thread access to a Tcl_Obj struct is at best perilous because the Tcl_IncrRefCount and Tcl_DecrRefCount public routines are not routines at all, but macros. And they are macros written with the strong assumption that while they operate the caller has exclusive access to the field. The only safe approach is to follow a strict rule that any Tcl_Obj is accessed by only a single thread that can be said to "own" that value. If you need to pass a value from one thread to another, you need to have the second thread create its own copy of the first, quite possibly limited only to the mechanism of making a full copy of the string rep. These rules are checked by the Tcl core when the --enable-symbols=mem configuration setting is on. Tcl_Obj allocation stores the allocated pointer in a thread-local hash map, and deallocation removes from the same thread-local hash map. Some mismatches (but not all?!?!) are detected with a Tcl_Panic(). | |||
User Comments: |
dgp added on 2018-04-03 12:45:07:
There's a far bigger concern about general sharing of a Tcl_Obj struct by multiple threads, though, and it is the volatility of the internal representation. Shimmering. Any code that interacts with an internal rep commonly has a structure of using objPtr->typePtr to determine the routines to use to act on the internal rep. Those routines proceed on the belief the internal rep's nature is not going to change while the routine runs. All internal rep access and manipulation is written on a foundational assumption of single-thread access. If you get multiple threads passing through changing intrep details out from under each other, there is going to be serious trouble. dgp added on 2018-04-03 12:38:03: The history of the objThreadMap restriction starts with these checkins by Mo Dejong. First he tried adding a field to the Tcl_Obj struct: https://core.tcl.tk/tcl/info/50c5d9ac11c96d30 And then followed up with the objThreadMap approach: https://core.tcl.tk/tcl/info/4dc4a16832209a53 The checkin and ChangeLog comments indicate the worry was about races between multiple threads manipulating the refCount field. This is a valid concern, but with mutex care and sufficient thread isolation it can be managed without imposing such a strong constraint. sebres added on 2018-04-03 08:56:04: > without any regard to whether the code is running in the same thread that created that Tcl_Obj. Why it is expected? The access to the objects in the tsv-storage is mutex-protected, and each set as well as the result of any tsv-call should always contain a safe duplication of the object (thus safe for any thread). BTW I cannot reproduce the segfault with your example. dgp added on 2018-03-30 11:50:17: In tclUtil.c, there's a collection of code supporting ProcessGlobalValues, which is one scheme for values to be shared among multiple threads. Maybe it is useful as inspiration or as a foundation? Maybe some of its routines would be useful to export? dgp added on 2018-03-29 23:08:52: The root of the problem is that the command procedures of many tsv commands contain Tcl_DecrRefCount(svObj->tclObj); without any regard to whether the code is running in the same thread that created that Tcl_Obj. Near as I can tell this entire sub-package is built on a misuse of the core. dgp added on 2018-03-29 23:04:16: With that background information, and using a --enable-symbols=mem build of Tcl, run this demo script: package require Thread proc myScript mainTid { package require Thread thread::send -async $mainTid [list puts "ThreadRunning = [tsv::get sharedArray ThreadRunning]"] tsv::set sharedArray ThreadRunning 1 thread::send -async $mainTid [list puts "ThreadRunning = [tsv::get sharedArray ThreadRunning]"] } proc installProc {tid procName} { set procBody [info body $procName] set procArgs [info args $procName] thread::send $tid [list proc $procName $procArgs $procBody] } tsv::set sharedArray ThreadRunning 0 set _threadHandle [thread::create] installProc $_threadHandle myScript thread::send -async $_threadHandle [list myScript [thread::id]] after 1000 puts "\t Completed tests" With any luck, one of those Panic safeguards will be triggered. |