fixed reader logic to support chunked messages

[vlad-diachenko]

Fixed reader logic to support the case when between chunks of one message the reader receives the chunks from another message or not chunked message.
more info grafana/loki#5644

Notes for Reviewers

• The commit history must be preserved - please use the rebase-merge or standard merge option instead of squash-merge
• Sync up with the author before merging

[simonswine]

Thanks for working on this. I have put a couple of thoughts together testing/reading to your code.

In general I feel you would need to add a couple more comments to make sure the code is easily understood/read at a later point. So for example make it clear that the 128 fragment limit is coming from the protocol itself: https://docs.graylog.org/docs/gelf

I also think there are a couple of edge cases that we need to cover, as pretty much anything can happen to UDP packets on the network. I would also love to see cases with modified data covered by the tests.

Nit: I also wonder if "assembler" is the best name, I think "defragmentation" is quite common thinking about networking protocols.

[simonswine]

Thanks for incorporating my feedback. I think there are only minor things now. See my suggestions inline.

I think it's also worth running golangci-lint. As those look all relevant:

gelf/reader_test.go:185:8: ineffectual assignment to err (ineffassign)
        conn, err := net.DialUDP("udp", nil, addr)
              ^
gelf/reader_test.go:74:4: SA5001: should check returned error before deferring conn.Close() (staticcheck)
                        defer conn.Close()
                        ^
gelf/reader_test.go:186:2: SA5001: should check returned error before deferring conn.Close() (staticcheck)
        defer conn.Close()
        ^
gelf/reader.go:91:2: SA4006: this value of `cBuf` is never used (staticcheck)
        cBuf := make([]byte, ChunkSize)

[vlad-diachenko]

Yep, thanks. I have fixed all issues in reader and reader_test files.
The rest of the issues are not related to this PR. I decided to not fix it here to not add redundant changes to this RP.

 golangci-lint run
gelf/tcpreader.go:115:19: Error return value of `conn.SetDeadline` is not checked (errcheck)
                conn.SetDeadline(time.Now().Add(2 * time.Second))
                                ^
gelf/udpwriter_test.go:244:12: Error return value of `io.Copy` is not checked (errcheck)
        go io.Copy(ioutil.Discard, r)
                  ^
gelf/udpwriter_test.go:252:17: Error return value of `w.WriteMessage` is not checked (errcheck)
                w.WriteMessage(&Message{
                              ^
gelf/udpwriter_test.go:270:12: Error return value of `io.Copy` is not checked (errcheck)
        go io.Copy(ioutil.Discard, r)
                  ^
gelf/udpwriter_test.go:278:17: Error return value of `w.WriteMessage` is not checked (errcheck)
                w.WriteMessage(&Message{
                              ^
gelf/udpwriter_test.go:296:12: Error return value of `io.Copy` is not checked (errcheck)
        go io.Copy(ioutil.Discard, r)
                  ^
gelf/udpwriter_test.go:304:17: Error return value of `w.WriteMessage` is not checked (errcheck)
                w.WriteMessage(&Message{
                              ^
gelf/writer.go:21:2: `addr` is unused (structcheck)
        addr     string
        ^
gelf/writer.go:23:2: `hostname` is unused (structcheck)
        hostname string
        ^
gelf/writer.go:25:2: `proto` is unused (structcheck)
        proto    string
        ^
gelf/tcpreader.go:13:2: `conn` is unused (structcheck)
        conn     net.Conn
        ^

[simonswine]

Yep, thanks. I have fixed all issues in reader and reader_test files. The rest of the issues are not related to this PR. I decided to not fix it here to not add redundant changes to this RP.

Yes do not worry about pre-existing linting issues 🙂

[simonswine]

Nice one. Thanks for addressing my suggestions so patiently 🙂

LGTM 🚀