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

Weird caching issue with our new _request implementation #14

Closed
michael opened this issue Jul 29, 2012 · 11 comments
Closed

Weird caching issue with our new _request implementation #14

michael opened this issue Jul 29, 2012 · 11 comments

Comments

@michael
Copy link
Collaborator

michael commented Jul 29, 2012

@mattpass we've got a crazy crazy caching problem with our current XMLHttpRequest native implementation. I couldn't find out what's exactly happening here.

You need to use the updated github.js that I haven't commited yet because of this bug:

https://gist.github.com/3196019

The error can be reproduced like so:

var repo = github.getRepo("michael", "cmake_cdt7_stalled");

// Triggers GET: https://api.github.com/repos/michael/cmake_cdt7_stalled/git/refs/heads/prose-integration
repo.getRef('obu_cdt7_support', function(err, ref) {
  // Triggers DELETE: https://api.github.com/repos/michael/cmake_cdt7_stalled/git/refs/heads/prose-integration
  repo.deleteRef("heads/obu_cdt7_support", function(err) {
    // For some weired reason the delete request doesn't get started, request inspection in chrome reveals
    // it just returns the content from the previous GET request (from cache)
  });
});

That's what the request log looks like for the DELETE request. Expected is a 204 no-content (not from cache) that actually removes the ref but we get a 200 OK (from cache). Seems like the variation of the method (GET vs. DELETE) is not considered for the caching mechanism.

Request URL:https://api.github.com/repos/michael/cmake_cdt7_stalled/git/refs/heads/prose-integration
Request Method:DELETE
Status Code:200 OK (from cache)
Request Payload
{"user":"michael","name":"cmake_cdt7_stalled"}
@michael
Copy link
Collaborator Author

michael commented Jul 29, 2012

@mattpass

Can't be sure but maybe jquery is adding an anti-cache fragment to the url to solve this issue... ?

https://github.com/jquery/jquery/blob/master/src/ajax.js#L632

Did I mention how much I hate HTTP? Oh and caching. ;)

Maybe I should just bring back jQuery... this thing is blocking a feature unfortunately.. and I've spent 3 hours of debugging last Friday. :(

@mattpass
Copy link
Collaborator

Mmm. Does this actually work with the jQuery AJAX version?

I'd approach this scientifically by testing the sam operagion 5 times in a row using XHR method and then 5 times in a row using jQuery method.

If you get odd results with XHR but the jQuery method works fine every time I'd look at why that is.

First step is confirm that I reckon.

@mattpass
Copy link
Collaborator

Something else I noticed is you're initialising 2 functions on the same line, ie the del func nested inside the initialisation of the get func. I'm not 100% sure if I've got this right, but doing it this way may be starting them both at the same time and under the same ref ID?

Side note: The xhr.open line has 2 params - for the method & URL. It does actually take a 3rd param for async behaviour or not via a true/false value. I've omitted this to go with the default of true, so it's not going to wait for a response and plough ahead with more code while a response is due to come back.

So you have 2 xhr calls which may be triggered at the same time and because it's using async, the get call certainly isn't going to wait for a response from the del call and the responses may get lumped into the same ref ID.

Sounds possible?

What happens if you run them independently of each other and only do the get call when we have a response from the del call?

@michael
Copy link
Collaborator Author

michael commented Jul 30, 2012

Mhh i think it shouldn't... The first call waits until the response is there.. then the second thing gets called. But let me double check. I'll also get in place a jquery $.ajax-based _request version so I can test the differences...

@michael
Copy link
Collaborator Author

michael commented Jul 30, 2012

https://developer.mozilla.org/en/using_xmlhttprequest

Read: Bypassing the cache

Man, this explains why a thing like jQuery makes sense.

@mattpass
Copy link
Collaborator

Did not know Gecko uses the cache by default. Wonder if other browsers do too? I'd go with the timestamp idea to make it x-browser, which is what jQuery appears to do.

Maybe something like this would work to add a timestamp with a random number appended and make URLs different therefore avoiding the cache:

var rand = +new Date + Math.floor(Math.random()*1e20);
url += url.indexOf('?') ? '&' : '?';
url += 'rand='+rand;

@michael
Copy link
Collaborator Author

michael commented Jul 30, 2012

Yeah, I'm going with that:

      function getURL() {
        var url = API_URL + path;
        return url + ((/\?/).test(url) ? "&" : "?") + (new Date()).getTime();
      }

works.

@mattpass
Copy link
Collaborator

Ah good. So this solves the problem then?

@michael
Copy link
Collaborator Author

michael commented Jul 30, 2012

Yeah. It does. Not quite happy about adding all those timestamps, but that's the only safe way it seems.

Why can't browser's caching implementations not just consider the combination of method + url for uniqueness? Would save millions of people headaches. ;)

@mattpass
Copy link
Collaborator

Or better still not cache at all. ;D

@michael
Copy link
Collaborator Author

michael commented Jul 30, 2012

Yeah. Always when I get cached data I feel kind of fooled. And with a reason! ;)

punchagan pushed a commit to punchagan/github that referenced this issue Aug 23, 2014
add getRepos call for organisations.
aendra-rininsland pushed a commit to aendra-rininsland/github that referenced this issue Oct 23, 2015
aendra-rininsland pushed a commit that referenced this issue Oct 23, 2015
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