Inline port numbers in URLs cause request to spawn multiple callback chains which loop "endlessly" #98

Closed
rahrens opened this Issue Oct 26, 2011 · 3 comments

Comments

Projects
None yet
2 participants
@rahrens

rahrens commented Oct 26, 2011

I've been using request for a small project where we crawl some pages on a site I work on. We noticed an issue where request was entering an "endless" loop when attempting to follow a redirect.

Further investigation showed that

  • request wasn't just looping until it hit options.maxRedirects. Instead it was looping until it hits options.maxRedirects, options.maxRedirects + 1 times over!
  • The issue occurs at the point when request fetches http://app.compete.com/login from our server and receives a 301 response redirecting it to https://app.compete.com:443/login
  • It's this second url, with an inline port which is actually confusing request. In fact, you don't even need to follow the redirect in order to observe the behavior -- you can just try to request https://app.compete.com:443/login directly.

I've attached some code demonstrating this behavior here: https://gist.github.com/1316008

I've also attached a sketch-of-a-patch. It appeared that by passing the same object around everywhere as options to callbacks that various callbacks were being run multiple times and that multiple chains of processing were running. Stopping that behavior by copying only the necessary properties into a new Object of properties given to the http or https module caused the problem to cease -- even when the same Agent is passed through.

To be honest, I'm submitting this in a gist not in a pull request because I don't really have a thorough enough understanding of why this fixes things. That means I view it as a piece of diagnostic information not a viable fix in its own right.

@mikeal

This comment has been minimized.

Show comment Hide comment
@mikeal

mikeal Oct 26, 2011

Owner

a lot of changes in this code have rolled in over the last few days. the fact that you're still referencing options means you're using an older version.

also, @isaacs changed our url parsing/handling and i merged it just yesterday. could you please try out master HEAD and tel me if this is still an issue?

Owner

mikeal commented Oct 26, 2011

a lot of changes in this code have rolled in over the last few days. the fact that you're still referencing options means you're using an older version.

also, @isaacs changed our url parsing/handling and i merged it just yesterday. could you please try out master HEAD and tel me if this is still an issue?

@rahrens

This comment has been minimized.

Show comment Hide comment
@rahrens

rahrens Nov 3, 2011

Hi @mikeal. I saw that you made a number of changes recently. I'll verify whether they impact this issue in the coming week and report back to you.

rahrens commented Nov 3, 2011

Hi @mikeal. I saw that you made a number of changes recently. I'll verify whether they impact this issue in the coming week and report back to you.

@mikeal

This comment has been minimized.

Show comment Hide comment
@mikeal

mikeal Feb 18, 2012

Owner

assuming this is fixed.

Owner

mikeal commented Feb 18, 2012

assuming this is fixed.

@mikeal mikeal closed this Feb 18, 2012

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