Conversation
| return "", -1, fmt.Errorf("btp: error writing bytes to file: %v", err) | ||
| } | ||
|
|
||
| // TODO: io.Copy panics |
There was a problem hiding this comment.
Making a note here.
This is something I would like to discuss when we meet next.
There was a problem hiding this comment.
io.Copy() panics because that function allocates its own buffer of size 32*1024 (https://golang.org/src/io/io.go?s=12621:12681#L388), which will likely be smaller than your total payload, and then calls read -> write multiple times until the read stream is exhausted. Here you are allocating one byte slice that is the size of your entire payload (which could be multiple megabytes).
The io.Copy() is a more efficient approach since it will only buffer one chunk at a time in memory, and that chunk size is only 32k, not multiple megabytes.
| } | ||
| err = cmd.Start() | ||
| if err != nil { | ||
| w.WriteString(fmt.Sprintf("error starting command: %v\n", err)) |
There was a problem hiding this comment.
No need to have the \n on this. This was left over from a copy/paste.
drstearns
left a comment
drstearns
left a comment
There was a problem hiding this comment.
Releasing the comments I have so far so that you can see them before our meeting. The big problem is in your Reader.Read() method. If I have more time before our meeting I'll review the rest!
| } | ||
| if rb.beenSent { | ||
| return fmt.Errorf("btp: cannot sent a request that has already been sent") | ||
| } |
There was a problem hiding this comment.
You sure you want to do this? What if you want to record previously-sent requests so that you can re-send them, similar to Postman?
| rb.response = conn | ||
| rb.beenSent = true | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Should you maybe return conn instead of setting the .response field? What happens if the calling code never touches that .response field? Seems like the API would be less error-prone if you return the conn as an io.ReaderCloser, so that the client knows it has to close that response stream after reading the response.
| cummBytesRead := 0 | ||
| chunkSize := 1024 | ||
|
|
||
| for int64(cummBytesRead) < r.size { |
There was a problem hiding this comment.
This isn't the right approach. Your job here is to just delegate to r.reader and decrement size until it reaches zero, at which time you start returning io.EOF. len(dst) could be less than r.size. If so, this code will try to copy more bytes than dst has length, and will thus panic.
| []byte(""), | ||
| }, | ||
| { | ||
| newReader(4, bytes.NewReader([]byte("pink fluffy ponies make me happy"))), |
There was a problem hiding this comment.
This is good to know about you!
| } | ||
| } | ||
|
|
||
| func TestMin(t *testing.T) { |
There was a problem hiding this comment.
I applaud your commitment to testing, but a min() function might not require a test suite!
| return "", -1, fmt.Errorf("btp: error writing bytes to file: %v", err) | ||
| } | ||
|
|
||
| // TODO: io.Copy panics |
There was a problem hiding this comment.
io.Copy() panics because that function allocates its own buffer of size 32*1024 (https://golang.org/src/io/io.go?s=12621:12681#L388), which will likely be smaller than your total payload, and then calls read -> write multiple times until the read stream is exhausted. Here you are allocating one byte slice that is the size of your entire payload (which could be multiple megabytes).
The io.Copy() is a more efficient approach since it will only buffer one chunk at a time in memory, and that chunk size is only 32k, not multiple megabytes.
| ) | ||
|
|
||
| func main() { | ||
| addr := "localhost:8080" |
There was a problem hiding this comment.
Is this the agent? If so, you might want to find an open port dynamically so that you can run on machines where port 8080 is already in-use (e.g., running a live-server or a React webpack dev server). See https://stackoverflow.com/a/43425461
2 participants
Implements a basic library for listening for and making BTP requests.