ugorji / go

idiomatic codec and rpc lib for msgpack, cbor, json, etc. msgpack.org[Go]
MIT License
1.85k stars 295 forks source link

encoding panics if writer returns an error on flush #285

Closed hsanjuan closed 5 years ago

hsanjuan commented 5 years ago

Since ce1d12656603bc8fa8009023db97a0f0176ca4b9, failing to write bytes during encode results in panics (where before it would error). In particular, writing to an unexpectedly closed connection now triggers a panic out of the blue.

This seems a pretty serious regression. One cannot always control if writing to a writer is going to work. If not, there should be an error, not a panic. As of now the only way to work with this library is using recover every time there is an Encode call.

hsanjuan commented 5 years ago

Particular panic comes from flush() at https://github.com/ugorji/go/blob/master/codec/encode.go#L331

ugorji commented 5 years ago

I need a reproducer please.

hsanjuan commented 5 years ago
package main

import (
    "bufio"
    "errors"
    "fmt"

    "github.com/ugorji/go/codec"
)

type errWriter struct {
}

func (ew *errWriter) Write(p []byte) (int, error) {
    return 0, errors.New("error writing")
}

func main() {
    writer := bufio.NewWriterSize(&errWriter{}, 5000)
    enc := codec.NewEncoder(writer, &codec.JsonHandle{})
    for {
        err := enc.Encode("hello")
        if err != nil {
            fmt.Println(err)
            break
        }
    }
}

It seems to happen when the writer is wrapped in a bufio.Writer.

ugorji commented 5 years ago

Thanks.

I've reproduced it, and your analysis is right. I have to figure out the best way to handle this.

We want to ensure that an error during flush is captured, but we also know that flush may not have been called by the time an error is captured. So we have to be delicate about it.

Let me think through the appropriate fix and follow up soon.

ugorji commented 5 years ago

Again, thanks for the bug report and the quick reproducer.

hsanjuan commented 5 years ago

Thanks for the quick fix!