Skip to content

TS-4396: fix number_of_redirections off-by-one#786

Closed
mingzym wants to merge 1 commit intoapache:masterfrom
mingzym:TS-4396
Closed

TS-4396: fix number_of_redirections off-by-one#786
mingzym wants to merge 1 commit intoapache:masterfrom
mingzym:TS-4396

Conversation

@mingzym
Copy link
Copy Markdown
Member

@mingzym mingzym commented Jul 2, 2016

please test

@zwoop zwoop added this to the 7.0.0 milestone Jul 2, 2016
Comment thread proxy/http/HttpSM.cc Outdated
}

redirection_tries++;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right below here, we repeat the test on number_of_redirections with is_redirect_required(). The condition is different however, since this code will not redirect withnumber_of_redirectionsandis_redirect_required()`` will.

I don't think that this is the right place to increment redirection_tries, though it is an improvement. How about incrementing it in redirect_request()? Or at least defer until we know we are actually going to call redirect_request.

We should restructure this function like this:

if (!is_redirect_required()) {
    tunnel.deallocate_redirect_post_buffers();
    enable_redirection = false;
    return;
}

...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, looks like better to do redirection_tries++ when setting redirect_in_process = true, if you consider a really redirection. good catch.

@zwoop zwoop modified the milestones: 7.1.0, 7.0.0 Sep 15, 2016
@PSUdaemon PSUdaemon requested a review from jpeach December 16, 2016 17:05
@PSUdaemon PSUdaemon assigned jpeach and unassigned jacksontj and sudheerv Dec 16, 2016
@zwoop zwoop modified the milestones: 7.1.0, 7.2.0 Jan 8, 2017
@zwoop zwoop modified the milestones: 7.2.0, 8.0.0 Apr 25, 2017
@PSUdaemon
Copy link
Copy Markdown
Contributor

[approve ci]

@bryancall
Copy link
Copy Markdown
Contributor

Closing since there is a new PR for this #2092

@bryancall bryancall closed this Jun 8, 2017
@zwoop zwoop removed this from the 8.0.0 milestone Jul 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants