Closed ibice closed 1 year ago
LGTM, thanks! Would you mind squashing this down to 1 commit?
Done @mreiferson!
hmm flaky test, not sure if it's really this PR's fault ...
=== RUN TestProducerHTTPConnectionFails
producer_test.go:275: should detect unexpected HTTP response, but got err: failed to IDENTIFY - write tcp 127.0.0.1:45031->127.0.0.1:4151: write: broken pipe
--- FAIL: TestProducerHTTPConnectionFails (0.00s)
Hi! I've replicated the failing test locally (Ubuntu 22.04 on amd64) using these commands:
# Fetch same ref as actions/checkout
cloneDir=$(mktemp -d)
git clone git@github.com:nsqio/go-nsq.git $cloneDir && cd $cloneDir
git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin \
+1220a820b387e21a42c5e798b8798a4a887c0f33:refs/remotes/pull/353/merge
git checkout --progress --force refs/remotes/pull/353/merge
# Download NSQ
curl -sSL "http://bitly-downloads.s3.amazonaws.com/nsq/nsq-1.1.0.linux-amd64.go1.10.3.tar.gz" \
| tar -xzv --strip-components=1
# Run tests
docker run \
-v $PWD:/go/src/github.com/nsqio/go-nsq \
-w /go/src/github.com/nsqio/go-nsq \
golang:1.17-buster \
/bin/bash -c 'export PATH=bin:$PATH && ./test.sh'
The output shows that the test does not fail:
[...]
=== RUN TestProducerHeartbeat
--- PASS: TestProducerHeartbeat (1.21s)
=== RUN TestProducerHTTPConnectionFails
--- PASS: TestProducerHTTPConnectionFails (0.00s)
PASS
ok github.com/nsqio/go-nsq 4.423s
killing nsqd PID 17
killing nsqlookupd PID 10
I've run it in my machine 20 times and it succeeded in all of them, so I suspect the error has something to do with the GitHub environment. I'll see if I can debug it in my go-nsq fork. In the meantime, if you could rerun the failed test, just to discard an spurious error (even if it has already failed twice), it could be of help.
thanks!
Add MaxMsgSize to configuration mimicking the nsqd cofiguration key. It defaults to 1048576 as the nsqd config -- source: https://github.com/nsqio/nsq/blob/a4939964f6715edd27a6904b87c2f9eb6a45e749/nsqd/options.go#L130
Pass MaxMsgSize to ReadResponse via its caller, ReadUnpackedResponse. Check msgSize does not exceed the maximum in ReadResponse. This reduces the risk of attempting to read arbitrary (non-nsq) responses.
Generate custom error if msgSize is the result of deserializing the first 4 bytes of an HTTP response (1213486160) to facilitate troublehooting
Fixes #352