added option for dialer to follow redirects

[NivKeidan]

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]

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]

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]

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]

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]

Kindly ping. Looks like it can be closed now.