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

added option for dialer to follow redirects #125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NivKeidan
Copy link

Hey guys,
First of all, thank you for this package! Great job!

This PR introduces ability for a client to follow redirects.

Protocol wise, following redirects is acceptable.
Previous behaviour is unmodified

Usage:
Simply add dialer.FollowRedirects = true

This is quite a naive implementation.
If you feel changes are required to make it more acceptable, I would happily put the time in.

@gobwas
Copy link
Owner

gobwas commented Jan 12, 2021

Hi @NivKeidan!

Thank you for such a good feedback!

At the first glance it looks like that following redirects can be done via Dialer.OnHeader callback the same way you did. I'm not sure yet which one is better, but I can share few of my thoughs on this.

Dialer might also want to count number of redirects done previously (as standard http.Client does and fails request after 10 consecutive redirects). Current implementation may introduce some for loop inside ws.Dialer.Dial() method.

However, all of the logic also can be implemented in separate function/method with use of Dialer.OnHeader field like so:

func DialWithRedirects() {
    d := ws.Dialer{
        OnHeader: func(...) error {
            // if header is location 
            return errRedirect 
        },
    }
    for n := 0; n < 10; n++ {
        if err := d.Dial(...); err == errRedirect {
            continue
        }
    }
    return errTooManyRedirects
}

But I'm not sure this should be done this way.

Also, I think you did actually break the previous behaviour since before the OnHeader callback was invoked with Location header. So if some of the users implement redirects using OnHeader, changing this will break their apps?

Let's think/discuss this a bit and then make a decision?

upd.

Looks like it's false alarm by me because previously we fail Dial() on non 101 status code.

@gobwas
Copy link
Owner

gobwas commented Jan 12, 2021

So I think it's good to have this right on Dialer struct, but with ability to limit the number of redirects done.
Probably we can also add some field like CheckRedirect as it is in http.Client and have the default version of it checking the size of requests already done.
Also please take a look on http.redirectBehavior function which has some useful tips on what we can do here as well.
Probably we can do this when checking resp.status:

switch {
    case resp.status == 101:
        break //  OK
    case d.FollowRedirects && isRedirect(resp.status):
        if urlstr, err = lookupLocationHeader(...); err != nil {
            return
        }
        if err := d.checkRedirect(urlstr, n); err != nil {
            return err
        }
        continue
    default:
        err = StatusError(...)
}

@NivKeidan
Copy link
Author

NivKeidan commented Jan 13, 2021

Hey @gobwas thanks for the input!

I will take a look at CheckRedirect and implement a default 10 retry limit.
I will also adopt this switch usage, it is much better.

I hope i'll get to it over the weekend!

@gobwas
Copy link
Owner

gobwas commented Jul 10, 2021

Hey @NivKeidan, I'm friendly asking if you are going to continue work on this? ) It's okay if not, I will just close this and maybe implement it on my own on my free time :)

@cristaloleg
Copy link
Collaborator

Kindly ping. Looks like it can be closed now.

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

Successfully merging this pull request may close these issues.

3 participants