New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mapraster.c: problematic dereferencing of kerneldensity dataset #5330

Closed
gogglesguy opened this Issue Sep 20, 2016 · 1 comment

Comments

Projects
None yet
2 participants
@gogglesguy
Contributor

gogglesguy commented Sep 20, 2016

Summary:
The in-memory kernel density heatmap gdal dataset handle (hDS) gets re-used but the associated rasterband cache doesn't get flushed, which causes old data to be rendered, instead of the newly generated imagery.

We've been testing the new Kernel Density Heatmap feature in mapserver 7.0. One major issue we've been seeing is that after running for a while mapserver starts returning incorrect heatmap data to our WMS requests. Eventually, it will be become pretty bad when just panning the map will return the exact same image back as the previous view. This made gave us the suspicion we were getting old cached data somehow.

This is has been easily reproducible with gdal < 2.0.
In gdal 2.0 and above I can't reproduce due to changes in the Memory driver that may have inadvertently fixed this issue in mapserver.

The situation:

msDrawRasterLayerLow calls msComputeKernelDensityDataset and receives a hDS handle. By default this handle only gets dereferenced ("DEFER" seems to be the default). After it is dereferenced, the memory attached to this dataset gets freed in msCleanupKernelDensityDataset. However the rasterband cache doesn't get flushed. This causes rendering issues later on.

This is technically just wrong. If you delete the underlying data, you can't just keep hanging on to the dataset handle.

msComputeKernelDensityDataset uses GDALOpenShared to return a new dataset. We've noticed that the longer it runs, the more likely a previous GDALDataSet is returned due to the fact that the memory will be allocated at the same location in memory for the same image size. See the following from one our logs where subsequent request used the same dataset:

MEM:::DATAPOINTER=0x82f48c0,PIXELS=1420,LINES=1098,BANDS=1,DATATYPE=Byte,PIXELOFFSET=1,LINEOFFSET=1420
MEM:::DATAPOINTER=0x82f48c0,PIXELS=1420,LINES=1098,BANDS=1,DATATYPE=Byte,PIXELOFFSET=1,LINEOFFSET=1420

Proposed solution is to always close the dataset handle in case of MS_KERNELDENSITY. It is the cleaner approach, especially since the underlying data does get deleted.

if (layer->connectiontype == MS_KERNELDENSITY) {
GDALClose(hDS);
}
else { /* previous dereference logic */ }

Alternatively, a call to GDALFlushCache(hDS) could help as well.

@gogglesguy gogglesguy changed the title from mapraster.c: problematic deferencing of kerneldensity dataset to mapraster.c: problematic dereferencing of kerneldensity dataset Sep 20, 2016

gogglesguy added a commit to gogglesguy/mapserver that referenced this issue Sep 21, 2016

tbonfort added a commit that referenced this issue Sep 21, 2016

Fix memory/locking errors on kerneldensity layers (#5330)
- Use GDALClose instead of GDALDereference for temporary in-memory datasets
- Properly aquire/release GDAL thread locks
@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Sep 21, 2016

Member

Thanks for the investigation and fix @gogglesguy . Applied to branch-7-0 in 3493927.

I was wondering if we shouldn't be explicitly setting the CLOSE_CONNECTION flag on the KD layer to avoid polluting mapraster.c with outside checks, but there are already many specific hooks in that file, so went with your fix as is.

Member

tbonfort commented Sep 21, 2016

Thanks for the investigation and fix @gogglesguy . Applied to branch-7-0 in 3493927.

I was wondering if we shouldn't be explicitly setting the CLOSE_CONNECTION flag on the KD layer to avoid polluting mapraster.c with outside checks, but there are already many specific hooks in that file, so went with your fix as is.

@tbonfort tbonfort closed this Sep 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment