#339 closed defect (fixed)
Thread-safety issue un setSymbolSet()
Reported by: | dmorissette | Owned by: | |
---|---|---|---|
Priority: | highest | Milestone: | 4.4 release |
Component: | MapScript-PHP | Version: | 4.1 |
Severity: | normal | Keywords: | |
Cc: | sgillies@… |
Description
Subject: RE: [Mapserver-dev] MS concurrency problem with vector symbols (partially solved) Date: Thu, 5 Jun 2003 13:42:23 +0100 From: "Mark Cave-Ayland" <m.cave-ayland@webbased.co.uk> To: <woodbri@swoodbridge.com>, <mapserver-dev@lists.gis.umn.edu> Hi Steve, > -----Original Message----- > From: mapserver-dev-admin@lists.gis.umn.edu > [mailto:mapserver-dev-admin@lists.gis.umn.edu] On Behalf Of > woodbri@swoodbridge.com > Sent: 04 June 2003 23:28 > To: mapserver-dev@lists.gis.umn.edu; Mark Cave-Ayland > Subject: Re: [Mapserver-dev] MS concurrency problem with > vector symbols > > > Mark, > > Mapserver, and therefore Mapscript, are not yet thread-safe. The > developers have been making a lot of changes to the source to remedy > this, and I don't think it is totally fixed in ms-3.7/4.0, let along > 3.6.5. > > You can search the archives for "thread safe" to get more info. By > the way, this is also the same reason that PHP must be built as a CGI > in Apache and not as a module if you want to use PHP/mapscript. I don't know whether I am imagining things but I seem to recall that a number of fixes went into the 3.6 branch a while back to deal with most of these issues? I've just had a look back over the archives for the past 6 months as you suggested, and the only outstanding issue I can find relates to the FreeType library (paths aren't too much of a problem as we use absolute paths for everything). Anyway, I've spent several hours chipping away at this one and I've managed to find where things were going wrong. The problem was that I was calling setSymbolSet() which is not thread safe because it uses the parser without locking. To fix the problem I implemented a lock around msLoadSymbolSet() in mapsymbol.c and also had to change the lexer reset code a little - please find the patch attached. I've load tested this with 4000 requests over 10mins and it hasn't failed once since the patch was applied. However.... This is not a great fix because it defines a new lock type TLOCK_SUBPARSER to allow the call to setSymbolSet() to work. Unfortunately this means that there is still a conflict with TLOCK_PARSER since an entire mapfile could possibly be lexed at the same time as the setSymbolSet() call is being executed since the locks are different and I don't think it is possible to set the same mutex twice without causing all sorts of problems :(. I think what mapthread.c is missing is a routine msCheckLock(int) which returns true if a lock has been taken. It would then be easy to modify the patch so that if TLOCK_PARSER was not already set then it would be set just for the duration of msLoadSymbolSet(). I've had a look at the pthread documentation but there was no obvious way to implement this. It would be great if the guy who wrote the code (Frank Warmerdam) could suggest a way around this one..... Many thanks, Mark. --- Mark Cave-Ayland Webbased Ltd. Tamar Science Park Derriford Plymouth PL6 8BX England Tel: +44 (0)1752 764445 Fax: +44 (0)1752 764446 This email and any attachments are confidential to the intended recipient and may also be privileged. If you are not the intended recipient please delete it from your system and notify the sender. You should not copy it or use it for any purpose nor disclose or distribute its contents to any other person.
Attachments (1)
Change History (14)
by , 21 years ago
Attachment: | mapserver-3.6.5-setsymbol.patch added |
---|
comment:1 by , 21 years ago
Owner: | changed from | to
---|
I briefly review the existing locking mechanism and there is no guarantee that a given lock can be multiple acquired by one thread in a safe "stack" style. I will spend some time today reviewing whether we can change the API to make this guarantee on all platforms.
comment:2 by , 21 years ago
dependson: | → 349 |
---|
comment:3 by , 21 years ago
Version: | 4.0 → 4.1 |
---|
Should be addressed at same time as bug 349 in version 4.1
comment:4 by , 21 years ago
Would it be possible to solve this by having a global array in mapthread.c that stores the PID (if any) of the process that has acquired the lock? msAcquireLock () and msReleaseLock() can be modified to insert the current PID into the array. (Of course all accesses to this array would need to be themselves protected with a mutex).
comment:5 by , 20 years ago
Frank, is this still an issue? Mark asked about this on mapserver-users last week.
comment:6 by , 20 years ago
Cc: | added |
---|
comment:7 by , 20 years ago
Milestone: | → 4.4 release |
---|---|
Status: | new → assigned |
I think this needs to be addressed for the 4.4 release which I hope will be more thread-safe ready than we have been in the past. I'm not exactly sure how I will handle it though.
comment:8 by , 20 years ago
A solutions exists for the originally reported problem in SWIG mapscript and I suggest that it be implemented for PHP mapscript. In the Perl/Python flavor of mapscript we have a constructor for instances of symbolObj and a method to append them to a symbolSetObj. The parser, therefore, is by-passed. Here's an example: >>> symbol = mapscript.symbolObj('new') >>> symbol_index = map.symbolset.appendSymbol(symbol) appendSymbol returns the index of the new symbol, and you'd specify this index in a style to use the symbol. This allows you to create symbols dynamically without any disk IO or use of the parser.
comment:9 by , 20 years ago
Sean, I think the issue here is with setSymbolSet() which is used to replace the current symbolset with a new symbolset. This necessarily requires going to disk and loading/parsing that symbolset. The SWIG MapScript $map->setSymbolSet() also ends up calling msLoadSymbolSet() so it's probably suffering of the same problem as PHP.
comment:10 by , 20 years ago
Owner: | changed from | to
---|---|
Priority: | high → highest |
Status: | assigned → new |
I'm helping Frank out by taking over this bug.
comment:11 by , 20 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Recent progress on thread safety: I've fixed trouble at three spots - parsing expression strings outside of msLoadMap - evaluating mapserver logical expressions - loading symbol set files outside of msLoadMap while leaving Frank's mapthread code unchanged. It's simple and works, no need to change it. Issues of thread safety can now be divided into two separate application phases: a "loading" phase and an "execution" phase, which is everything else. During the loading phase, we acquire a parser lock immediately and release it only after the entire mapfile has been loaded. Within this phase we will use parsing functions with no locking of their own, particularly loadExpressionString, and loadSymbolSet. Outside of the loading phase, we must use locking versions of these functions. msLoadExpressionString and msLoadSymbolSet are wrappers for the previous functions. I've also added parser locking to msEvalExpression and msEvalContext, and have changed all execution-phase code to use these parser locking functions. My tests pass, the code is working fine in my multithreaded Zope app, so I'm going to call it "FIXED". We are now safer, but still have a performance bottleneck. The one parser is shared between threads in mapfile loading and map rendering (or "execution") phases. In the first case a lock is held relatively long, while in the second case it's acquired and released (inside msEvalExpression) quickly with every call to msLayerNextShape. I believe the tendency would be for map rendering to be upstaged by mapfile loading when activity is high. I see at least three solutions that we may want to consider as a new tracker issue for 4.4: 1) implement a second global parser for use in msEvalExpression and/or 2) pull the parser locking out of msEvalExpression, and instead lock around the loops which call msLayerNextShape. or 3) rewrite msEvalExpression so that it doesn't need a parser for logical expressions.
comment:12 by , 20 years ago
Sean Gillies wrote: > 1) implement a second global parser for use in msEvalExpression Sean, My understanding (and Steve obviously knows more about this) is that the mapfile parsing uses only the lexer while msEvalExpression() uses the yacc/bison parser which in turn uses the same low level lexer as the mapfile code uses. It might be possible to build a seperate lexer instance for the expressions. > and/or > > 2) pull the parser locking out of msEvalExpression, and instead lock around > the loops which call msLayerNextShape. I don't think this would be all that wise. My take would be that mapfile parsing is pretty quick while in some cases processing shapes can take a long time, much of which is not spent in the parser at all. > or > > 3) rewrite msEvalExpression so that it doesn't need a parser for logical > expressions. > > I'd love to get some feedback from folks who are interested in using > mapserver in a multithreaded environment. Well, my longer term suggestion has always been to look into whether we can get a thread safe lexer in place, either by researching flex options in greater depth or if necessary by rewriting the lexer by hand. Lexers aren't really that hard.
comment:13 by , 20 years ago
The original issue of this bug is fixed. I have created bug 955 to discuss the possible performance enhancements. Let's continue the discussion there.
Note:
See TracTickets
for help on using tickets.
mapserver-3.6.5-setsymbol.patch - The patch mentioned in this bug