Opened 13 years ago
Last modified 13 years ago
#3692 reopened defect
msLayerClose expression freeing "too vigorous"
Reported by: | warmerdam | Owned by: | sdlime |
---|---|---|---|
Priority: | normal | Milestone: | 6.0 release |
Component: | MapServer C Library | Version: | svn-trunk (development) |
Severity: | blocker | Keywords: | query |
Cc: | dmorissette, tamas |
Description
Steve,
I have a python mapscript test suite script (msautotest/mspython/rqtest.py) that is failing. It seems it is because the script issues a raster query, closes the layer and then issues a new query.
The problem is that closing the layer frees the layer->class[]->expression.string and other information about the expression that is needed to evaluate the expression properly.
The relavent code in msLayerClose() is:
/* clear out items used as part of expressions (bug #2702) -- what about the layer filter? */ for(i=0; i<layer->numclasses; i++) { freeExpression(&(layer->class[i]->expression)); freeExpression(&(layer->class[i]->text)); }
While the above code appears like it might have been in place for a while, it seems to be a problem for my script since the overhaul of the query engine. I'm seeing the problem with raster queries, but it seems like it could impact some vector query access patterns too.
Change History (15)
comment:1 by , 13 years ago
comment:2 by , 13 years ago
Hmmm... I'll have a look. The new expression handling really shouldn't be affected any differently than the older approach if the freeExpression() calls have really been in place for some time. The leaving of layers open at the end of a query operation (so the result set is still active) is a 5.6 change so I would expected this problem to show before these latest changes.
Steve
comment:3 by , 13 years ago
Steve,
Well, I will say that the msautotest/mspython scripts are rarely run. They might well have been broken for as much as a year.
comment:4 by , 13 years ago
Status: | new → assigned |
---|
comment:5 by , 13 years ago
Cc: | added |
---|
comment:6 by , 13 years ago
Bug #2702 recognized that we didn't want to be too vigorous. Not sure why I/we went to freeExpression() but clearly that's a mistake. I've broken freeExpression into two functions with a new freeExpressionTokens(). That function just touches the token linked list and leaves expression data not associated with a layer data source in tact. I'll try putting that in place instead.
We need to apply it in more places in msLayerClose() to avoid problems including the layer filter and style geomtransforms...
Steve
comment:7 by , 13 years ago
Cc: | added |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
Fixed. The rqtest.py script completes successfully and the scenario Tamas described also works now. Change committed in r11037. Closing for now.
Steve
comment:8 by , 13 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
I reopen this, because I found another scenario where it doesn't seem to work. For long running processes (mapscript) when using queryByRect first, and then use multiple drawQuery while keeping the layer open between those drawings (to utilize single pass). (Tested with OGR data sources)
comment:9 by , 13 years ago
Forgot to say that the classes should have expressions and the error is 'Failed to parse expression' in msEvalExpression.
comment:10 by , 13 years ago
Is there a specific test case available or is this just query, draw/save, draw/save?
Steve
comment:11 by , 13 years ago
Severity: | normal → critical |
---|
I've now run into the same problem. The test have been taken out from #3858 To reproduce this, you should make the union layer queryable and then call map.queryByRect to the map extent twice.
It seems that freeExpressiontokens should be called somewhere between the queries.
comment:12 by , 13 years ago
Sounds like a 6.0 blocker? Or should we keep this for 6.0.1 since it affects only union layers, a new feature in 6.0?
comment:13 by , 13 years ago
Severity: | critical → blocker |
---|
Though the sample is taken out from an union layer bug, this problem is not related to the union layer and could also be reproduced with other layer types. I think this should be fixed for 6.0. (severity raised)
comment:14 by , 13 years ago
Give me until tomorrow AM to replicate/fix... We may need to clear expression tokens for all layers at before executing a query. Fortunately there's a query wrapper function (msExecuteQuery) to do it one place. Bad news is mapscript doesn't use it, it's calling the functions directly.
Steve
comment:15 by , 13 years ago
Quick solution is to call msLayerClose() before msLayerOpen() in the various msQueryBy... functions. This makes sure everything is nice and clean before starting another query operation. This fixed the the back-to-back query issue Tamas reported at least with a regular polygon layer (the error was present before the change). I did not try union layers. Frank, are raster queries affected?
(As an aside the query, draw query, draw query scenario reported earlier in this ticket worked fine for me.)
All the msautotest/query tests pass (shapefile and PostGIS).
Committed in r11647... I'll leave this open pending confirmation on the raster side of things.
Steve
Note, the test failure looks like:
The raster query is being improperly classified because the class expressions have been cleared.