msLayerClose expression freeing "too vigorous"

[warmerdam]

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.

[warmerdam]

Note, the test failure looks like:

  TEST: rqtest_13 ... fail
    attribute red is "-255" instead of expected "255"

The raster query is being improperly classified because the class expressions have been cleared.

[sdlime]

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

[warmerdam]

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.

[sdlime]

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

[sdlime]

Fixed. The rqtest.py script completes successfully and the scenario Tamas described also works now. Change committed in r11037. Closing for now.

Steve

[tamas]

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)

[tamas]

Forgot to say that the classes should have expressions and the error is 'Failed to parse expression' in msEvalExpression.

[sdlime]

Is there a specific test case available or is this just query, draw/save, draw/save?

Steve

[tamas]

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.

[dmorissette]

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?

[tamas]

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)

[sdlime]

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

[sdlime]

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