>
> My thought then is that access to reads and writes on the req.Body must
> be serialized to avoid a race condition.
>
Could you elaborate on how you concluded that? Preserves, does not mean
'takes ownership' as I understand.
Using a mutex for this prevented the race conditions.
But this is not the solution. It would just ensure that we don't have race
on reading variables but we can still have 'read after reset/free' since
there is a possibility that `reader.Reset()` will happen before
`reader.Read` (you can easily check that by setting atomically variable in
`Reset` to 1 and check if its > 0, every time in `Read`). Also if you
uncomment the line in `Close` implementation you get another races which
suggest that we might have 'read after close'.
And why we should do that (introduce locking) in the first place. We are
not obligated to protect body in regular request but we need to protect it
during redirected request?! This is wrong. How does user (unaware of such
practice) can write safe code and fast code not knowing if the server will
redirect the request or not. Also notice that there is a race between
`reader.Reset` and `reader.Read` and `Reset` happens AFTER the request
should be already finished. Which suggest that there is some goroutine
in-flight (spawned by stdlib) which still tries to `Read` even though `Do`
already returned a response.
W dniu środa, 6 marca 2019 11:23:51 UTC+1 użytkownik Uzondu Enudeme napisał:
>
> According to the doc on client.Do, a 307 or 308 redirect preserves the
> original
> http method and body provided that the request.GetBody function is defined
>
> My thought then is that access to reads and writes on the req.Body must
> be serialized to avoid a race condition.
>
> Using a mutex for this prevented the race conditions.
>
> My own question then is, what is the implication of serializing such
> access.
> Wouldn't that have been the implementation if the stdlib was to make the
> req.Body
> safe during a redirect?
>
> - Uzondu
>
> > On 5 Mar 2019, at 3:23 PM, virr...@gmail.com <javascript:> wrote:
> >
> > I've been playing with server which redirects requests but then I
> noticed weird races.
> > What is odd that if these races are expected it would mean that using
> `*File` in `req.Body`, for requests which are redirected, is not safe -
> this is because we have races between `Read` and `Close`.
> >
> > I've run following script with: `go run -race concept.go`
> >
> > package main
> >
> > import (
> > "crypto/rand"
> > "fmt"
> > "io"
> > "io/ioutil"
> > "net/http"
> > "sync"
> > "time"
> > )
> >
> > const (
> > addr1 = "localhost:8060"
> > addr2 = "localhost:8061"
> > )
> >
> > type customReader struct {
> > iter int
> > size int
> > }
> >
> > func (r *customReader) Read(b []byte) (int, error) {
> > maxRead := r.size - r.iter
> > if maxRead == 0 {
> > return 0, io.EOF
> > }
> > if maxRead > len(b) {
> > maxRead = len(b)
> > }
> > n, err := rand.Read(b[:maxRead])
> > r.iter += maxRead
> > return n, err
> > }
> >
> > func (r *customReader) Close() error {
> > // Uncomment this for even more races :((
> > // r.iter = 0
> > return nil
> > }
> >
> > func (r *customReader) Reset() {
> > r.iter = 0
> > }
> >
> > func second(w http.ResponseWriter, r *http.Request) {
> > ioutil.ReadAll(r.Body)
> > }
> >
> > func first(w http.ResponseWriter, r *http.Request) {
> > http.Redirect(w, r, "http://"+addr2+"/second";,
> http.StatusTemporaryRedirect)
> > }
> >
> > func main() {
> > mux1 := http.NewServeMux()
> > mux1.HandleFunc("/first", first)
> > srv1 := http.Server{
> > Addr: addr1,
> > Handler: mux1,
> > }
> > go func() {
> > err := srv1.ListenAndServe()
> > fmt.Printf("err: %v", err)
> > }()
> >
> > mux2 := http.NewServeMux()
> > mux2.HandleFunc("/second", second)
> > srv2 := http.Server{
> > Addr: addr2,
> > Handler: mux2,
> > }
> > go func() {
> > err := srv2.ListenAndServe()
> > fmt.Printf("err: %v", err)
> > }()
> >
> > time.Sleep(time.Second)
> > client := http.DefaultClient
> >
> > wg := &sync.WaitGroup{}
> > for i := 0; i < 1000; i++ {
> > reader := &customReader{size: 900000}
> > wg.Add(1)
> > go func(r *customReader) {
> > for {
> > req, err :=
> http.NewRequest(http.MethodPut, "http://"+addr1+"/first";, r)
> > if err != nil {
> > fmt.Printf("%v", err)
> > }
> > req.GetBody = func() (io.ReadCloser,
> error) {
> > return &customReader{size:
> 900000}, nil
> > }
> >
> > if resp, err := client.Do(req); err !=
> nil {
> > // fmt.Printf("error: %v", err)
> > } else if resp.StatusCode >=
> http.StatusBadRequest {
> > // fmt.Printf("status code: %d",
> resp.StatusCode)
> > }
> >
> > // Reset reader and try to reuse it in
> next request
> > r.Reset()
> > }
> >
> > wg.Done()
> > }(reader)
> > }
> >
> > wg.Wait() // infinite wait
> >
> > srv1.Close()
> > srv2.Close()
> > }
> >
> >
> >
> > Races which are occuring:
> >
> > ==================
> > WARNING: DATA RACE
> > Write at 0x00c0003e00a0 by goroutine 87:
> > main.main.func3()
> > /home/januszm/concept.go:43 +0x55
> >
> > Previous write at 0x00c0003e00a0 by goroutine 3899:
> > [failed to restore the stack]
> >
> > Goroutine 87 (running) created at:
> > main.main()
> > /home/januszm/concept.go:91 +0x3b0
> >
> > Goroutine 3899 (finished) created at:
> > net/http.(*Transport).dialConn()
> > /usr/local/go/src/net/http/transport.go:1358 +0xb89
> > net/http.(*Transport).getConn.func4()
> > /usr/local/go/src/net/http/transport.go:1015 +0xd0
> > ==================
> > ==================
> > WARNING: DATA RACE
> > Write at 0x00c0002ca750 by goroutine 488:
> > main.main.func3()
> > /home/januszm/concept.go:43 +0x55
> >
> > Previous write at 0x00c0002ca750 by goroutine 1951:
> > main.(*customReader).Read()
> > /home/januszm/concept.go:33 +0x156
> > net/http.transferBodyReader.Read()
> > /usr/local/go/src/net/http/transfer.go:62 +0x77
> > io.copyBuffer()
> > /usr/local/go/src/io/io.go:402 +0x143
> > net/http.(*transferWriter).writeBody()
> > /usr/local/go/src/io/io.go:364 +0x76e
> > net/http.(*Request).write()
> > /usr/local/go/src/net/http/request.go:655 +0x7c3
> > net/http.(*persistConn).writeLoop()
> > /usr/local/go/src/net/http/transport.go:1961 +0x321
> >
> > Goroutine 488 (running) created at:
> > main.main()
> > /home/januszm/concept.go:91 +0x3b0
> >
> > Goroutine 1951 (running) created at:
> > net/http.(*Transport).dialConn()
> > /usr/local/go/src/net/http/transport.go:1358 +0xb89
> > net/http.(*Transport).getConn.func4()
> > /usr/local/go/src/net/http/transport.go:1015 +0xd0
> > ==================
> >
> > When L39 is uncommented, we also get:
> >
> > ==================
> > WARNING: DATA RACE
> > Write at 0x00c0001620e0 by goroutine 58:
> > main.(*customReader).Close()
> > /home/januszm/concept.go:39 +0x3a
> > net/http.(*Client).do()
> > /usr/local/go/src/net/http/request.go:1346 +0x6f2
> > main.main.func3()
> > /usr/local/go/src/net/http/client.go:509 +0x183
> >
> > Previous write at 0x00c0001620e0 by goroutine 245:
> > main.(*customReader).Read()
> > /home/januszm/concept.go:33 +0x156
> > net/http.transferBodyReader.Read()
> > /usr/local/go/src/net/http/transfer.go:62 +0x77
> > io.copyBuffer()
> > /usr/local/go/src/io/io.go:402 +0x143
> > net/http.(*transferWriter).writeBody()
> > /usr/local/go/src/io/io.go:364 +0x76e
> > net/http.(*Request).write()
> > /usr/local/go/src/net/http/request.go:655 +0x7c3
> > net/http.(*persistConn).writeLoop()
> > /usr/local/go/src/net/http/transport.go:1961 +0x321
> >
> > Goroutine 58 (running) created at:
> > main.main()
> > /home/januszm/concept.go:87 +0x3b0
> >
> > Goroutine 245 (running) created at:
> > net/http.(*Transport).dialConn()
> > /usr/local/go/src/net/http/transport.go:1358 +0xb89
> > net/http.(*Transport).getConn.func4()
> > /usr/local/go/src/net/http/transport.go:1015 +0xd0
> > ==================
> >
> >
> > --
> > You received this message because you are subscribed to the Google
> Groups "golang-nuts" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an email to golang-nuts...@googlegroups.com <javascript:>.
> > For more options, visit https://groups.google.com/d/optout.
>
>
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
