Skip to content
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

Unexpected data in rendered grid #130

Closed
strk opened this issue Dec 19, 2012 · 7 comments
Closed

Unexpected data in rendered grid #130

strk opened this issue Dec 19, 2012 · 7 comments

Comments

@strk
Copy link
Contributor

strk commented Dec 19, 2012

This is an hard-to-track bug we hit on cartodb: utf8 grids are returned which contain unexpected data (more visibly field names). CartoDB/Windshaft-cartodb#67

I've spent the whole day to figure out what's done where and I'm still not sure I got the whole flow, but what I'm looking at right now is src/mapnik_map.cpp and wondering if the C++ code shouldn't add reference counting to object arguments, in particular I'm thinking about the "interactivity" array parameter.

@springmeyer
Copy link
Member

Can you paste one? Sounds like a memory corruption issue perhaps, but it could be anything (grid code is pretty complex)

@springmeyer
Copy link
Member

not quite following what you mean by reference counting. Can you describe any more the situations you've seen this with?

@strk
Copy link
Contributor Author

strk commented Dec 19, 2012

See this one: http://erepublik.cartodb.com/tiles/regions/7/84/39.grid.json?interactivity=cartodb_id
Replace 39 with 30 in the URL (that's the Y) and you'll see "interactivity" honoured in output.
Y=31 also fails. It may have to do with the fact that the grid is empty.

@strk
Copy link
Contributor Author

strk commented Dec 19, 2012

By reference counting I meant something on the C side to keep alive JS objects. Don't think I know what I'm saying though, as I never played with extending node.js, so dunno if any such thing is needed. The concern is that we're passing a JS array to a C-implemented method (the Mapnik object constructor) and I wonder how does the GC knows that the object is still needed, given the metatiler uses it in a different tick that the one he receives it.

@springmeyer
Copy link
Member

re: reference counting, okay, yes makes sense. My understanding is that when you pass JS objects that wrap C++ data you need to reference count. So, for example a mapnik.Grid can be passed to the map.render() async function and it is reference counted here: https://github.com/mapnik/node-mapnik/blob/master/src/mapnik_map.cpp#L1468 to ensure it does not get garbage collected while being rendered to. In terms of other JS objects (pure V8 objects) they cannot be used safely in the thread pool, so they should all be being copied to C structures: https://github.com/mapnik/node-mapnik/blob/master/src/mapnik_map.cpp#L1275-L1286

Can you check and make sure you do not have multiple node-mapnik installs in your node_modules directory? Sometimes that can happen and will produce really bizarre results or crashes.

@strk
Copy link
Contributor Author

strk commented Dec 19, 2012

single node-mapnik install, version 0.7.16.

Ok so you're saying that the array of field names only hits C trough the "options" parameter which is only temporary hold (I see fieldnames strings copied here I think:
https://github.com/mapnik/node-mapnik/blob/master/src/mapnik_map.cpp#L1458

Then maybe it has to do with the fact that the grid is fully contained within a feature.. I'll continue debugging tomorrow.

@strk
Copy link
Contributor Author

strk commented Dec 20, 2012

We found the real culprit being the "solid" cache in tilelive-mapnik:
mapbox/tilelive-mapnik#55

@strk strk closed this as completed Dec 20, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants