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

Race condition with external resources #82

Closed
strk opened this issue Aug 13, 2013 · 16 comments
Closed

Race condition with external resources #82

strk opened this issue Aug 13, 2013 · 16 comments
Assignees
Labels

Comments

@strk
Copy link
Contributor

strk commented Aug 13, 2013

Sometimes the localized external resources needed by CartoCSS are removed from disk before their corresponding style expires.

This seems to be due to the way filesystem path for the resources is determined.
Originally reported here:
https://groups.google.com/forum/#!topic/cartodb/m9t4iFdM-ok/discussion

Jira ref: https://cartodb.atlassian.net/browse/CDB-67

@ghost ghost assigned strk Aug 13, 2013
@saleiva
Copy link

saleiva commented Aug 13, 2013

Hi @strk I don't think that's the problem.
Without touching the original resource applying the same cartocss to a map several times retrieves some errors randomly.

@strk
Copy link
Contributor Author

strk commented Aug 13, 2013

I tried locally and it looks like applying the same cartocss to a map doesn't really send any request to the tiler from the browser. Instead the browser performs a PUT to the maps API (rails). Indeed I do see a request to the deprecated API arriving to the tiler from the rails API host, so the problem could be there. Can you confirm there's no request to tiler from the browser on later requests ?

@strk
Copy link
Contributor Author

strk commented Aug 13, 2013

I confirm there's no request from browser. Tried online. The call to deprecated API is really triggered by the PUT to maps api.

@strk
Copy link
Contributor Author

strk commented Aug 13, 2013

So we have two options here:

@strk
Copy link
Contributor Author

strk commented Aug 13, 2013

I'm seeing more weird things here. Specifically I'm seeing multilayer map styles in redis that reference resources downloaded to per-table cache dirs. This may very well be the cause of our troubles. It could be that while copying the style from a so-called "table" we end up getting the localized path rather than the original path, which is then unguaranteed as per lifetime. \cc @lorenzoplanas @javisantana

@strk
Copy link
Contributor Author

strk commented Aug 14, 2013

@lorenzo, what about avoiding to POST to /style if the style didn't change ? According to @saleiva the problem can only be reproduced by continuously applying the same style, so avoiding the override might fix it and generally improve interaction speed. In any case, I cannot reproduce this bug :(

@strk
Copy link
Contributor Author

strk commented Aug 21, 2013

So it looks to me that millstone is really not meant to be used the way we are, that is with different configurations within the same process. Look here:
https://github.com/mapbox/millstone/blob/v0.6.0/lib/millstone.js#L72

That global has is used to keep track of "in progress" downloads.
When more requests come in for the same url, the second request is put in a queue and called when download completes:
https://github.com/mapbox/millstone/blob/v0.6.0/lib/millstone.js#L104

Now I suspect that this means accepting the local path for the first request and discarding the one specified in the second. I filed a ticket to that extent: tilemill-project/millstone#105

@strk
Copy link
Contributor Author

strk commented Aug 21, 2013

I don't see any easy way to force millstone to use local variables for those caches. This means we either fork millstone to avoid the cache or we change our approach to use a single sandbox with all localized resources together. If we take the "single sandbox" approach we'll have a problem deciding when it is safe to cleanup elements in the cache.

@strk
Copy link
Contributor Author

strk commented Aug 21, 2013

I've sent a pull request for millstone, but it would be helpful if anyone could confirm the pull fixes the bug (I can't handle to reproduce myself): tilemill-project/millstone#107

@strk
Copy link
Contributor Author

strk commented Aug 21, 2013

Testing this involves installing millstone from our fork: https://github.com/CartoDB/millstone/tree/master-sandboxes
millstone is a dependency of grainstore \cc @javisantana

@strk
Copy link
Contributor Author

strk commented Aug 22, 2013

Pull request was merged, so it's a matter of testing against millstone master now

@javisantana
Copy link
Contributor

will try it asap

@strk
Copy link
Contributor Author

strk commented Sep 4, 2013

tested, finally ?

@strk
Copy link
Contributor Author

strk commented Sep 4, 2013

I've added a test within grainstore: CartoDB/grainstore#52

@strk strk closed this as completed in 83883b5 Sep 4, 2013
@strk
Copy link
Contributor Author

strk commented Sep 4, 2013

Reopening, as there might be a problem with the new millstone version:
https://travis-ci.org/CartoDB/Windshaft/builds/10977881#L1830

@strk strk reopened this Sep 4, 2013
@strk
Copy link
Contributor Author

strk commented Sep 4, 2013

never mind, it was a time-related issue.will raise the timeouts

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

No branches or pull requests

3 participants