-
Notifications
You must be signed in to change notification settings - Fork 35
Refactor route handling and server #78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
a3bccf8
to
0ae220a
Compare
0ae220a
to
f20f82b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a welcome change -- have a routing system HTTP request multiplexer in place seems like a standard practice and should let us expand the capabilities of the extension further if needed.
No objections to landing this as but so approving, but I do have some small suggestion/questions below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple comments but that's all, looks good!
return | ||
} | ||
defer resp.Body.Close() | ||
body, err := ioutil.ReadAll(resp.Body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The body
shouldn't be massive in size, but a good habit is to operate on the resp.Body
as an io.Reader
, and use something like io.Copy
to keep memory allocation down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I'll change that 👍🏼
log.Println("Could not read bytes from agent request body") | ||
return | ||
} | ||
if r.Body == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you encounter the body being nil? I took a look at the docs because I was checking to see if you need to close the request body, and it says:
// For server requests, the Request Body is always non-nil
// but will return EOF immediately when no body is present.
// The Server will close the request body. The ServeHTTP
// Handler does not need to.
so I'm not sure if this nil check is required. I suppose it doesn't hurt, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the docs say it will never be nil
, it doesn't seem necessary. I'll remove it :)
While working on #62, I found that it was awkward to find a way to pass the channel signaling that the function completed to the router handler. In defining the handlers as they are in this PR, we can easily pass the channel to the handler function, without having to keep a reference to it somewhere.