Introduction of net/http concept #2254
Conversation
Dear MikaeelMFThank you for contributing to the Go track on Exercism! 💙
Dear Reviewer/Maintainer
Automated comment created by PR Commenter 🤖. |
|
Currently, I am planning to break the concept introduction into two major parts. One part explains how to create HTTP clients and another how to create HTTP servers. For the server section, I have not really decided yet on what to include and what not to include. I am still doing some reading on this topic to further familiarize myself with it so I can decide on a good set of subsections to explain it well. Also for the exercise, I am thinking about creating a backstory related to one of the previous exercises used in the roadmap but haven't really gotten into it since I first want to finish the concept introduction. I will create a To-Do list to show the progress and will update it as I update this PR. |
Concept introduction:
|
MikaeelMF
changed the title
|
Two things that I have left out are discussing Middleware and concurrency. Since middleware is a general concept and it can be written in form of handlers, I did not seem it necessary to include it in this introduction. Also on the concurrency, since it is a bit out of scope for this concept, I did not got into it. |
|
@MikaeelMF I did not read the current content yet, I will need to find some more time for that. I just wanted to comment on what you wrote above regarding middleware and concurrency. I agree middleware can be left out for now as there is no special support for this in plain Go anyway. Regarding concurrency, I think it is important to explain in the server part that net/http creates a new Go routines for each incoming request and because of that it is totally fine to have synchronous/blocking code in an http handler. A lot of Go beginners don't understand this properly and kick off additional unnecessary go routines inside the handler. |
|
You don't need configlet to change the root level config.json file, it won't do that for you anyway. You need to edit the config yourself and add the new concept to the concept array. You can generate the needed uuid with any online tool. |
|
@junedev I will update the text and add this section and will start to work on the exercise parts. Also I have one question regarding exercism code of conduct. Is it more appropriate if I write both concept and exercise in one PR or I should create a separate PR for the exercise part? |
|
Re concurrency Re splitting server/client exercises Re PRs Sidenote: A Code of Conduct is a special document that explains how to behave, not processes/practices. 🙂 https://exercism.org/docs/using/legal/code-of-conduct |
|
@junedev |
|
@MikaeelMF A concept can have many practice exercises that allow the student to use the learned concept later on but a concept can only have have one learning/concept exercise that introduces/teaches that concept. If you are referring to those bubbles in the Python concept boxes, those are practice exercises. If you click on one concept, there is always one exercise to "Learn X" on the top right. In the context of the issue you are working on, we don't consider practice exercises yet, we are referring to the one concept exercise. We can talk about creating more practice exercises later but for know we should focus on the task at hand. I know there is a lot to learn when creating the first concept/exercise here. It also took me quite a long time to wrap my head around things when I started with that. Just continue to ask questions if something is unclear. |
|
@junedev |
|
So I am at the final stages of writing this concept's exercise, I have some questions regarding how to implement some parts of it:
I would really appreciate it if I can get any help with these. |
The concept's You can read more about this in:
The same as the module name specified in Lines 51 to 61 in 13f72b3 |
There was a problem hiding this comment.
I reviewed the client concept. I will continue the review other parts some other time but wanted to already share my findings on the first bit. Feel free to already continue working on the client concept based on the comments but please refrain from making changes to the client exercise for now. I will share more thoughts on the exercise once I get there.
Co-authored-by: June <[email protected]>
|
@junedev |
|
|
||
| ~~~~exercism/caution | ||
| Draining the request body before closing it: | ||
| Unlike the client that drains the body before closing it, in the server you need to manually drain the request body. A good way to do so is writing a middleware that closes the request after the server job is done (I have seen this pattern for the first time in [Adam Woodbeck's mux_test.go][awoodbeck-gnp-chapter09-mux_test.go]): |
There was a problem hiding this comment.
I have never heard of this and I don't think it is correct.
The official documentation https://pkg.go.dev/net/http#Request states
// 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.
The behavior here is different then on the client side where you need to close the response body on your own.
There was a problem hiding this comment.
@junedev
Ok so after a lot of digging I did not find an answer that satisfies me regarding this issue.
First please take a look at this:
http://tleyden.github.io/blog/2016/11/21/tuning-the-go-http-client-library-for-load-testing/
I have tested it with go version 1.19.3 and it still behaves the same.
The response.Body.Close() will not drain the body therefore not allowing the same connection to get reused.
This is also another post discussing the same issue in 2017:
https://forum.golangbridge.org/t/do-i-need-to-read-the-body-before-close-it/5594
Also please take a look at here:
https://pkg.go.dev/net/http#Response.Close
// Body represents the response body.
//
// The response body is streamed on demand as the Body field
// is read. If the network connection fails or the server
// terminates the response, Body.Read calls return an error.
//
// The http Client and Transport guarantee that Body is always
// non-nil, even on responses without a body or responses with
// a zero-length body. It is the caller's responsibility to
// close Body. The default HTTP client's Transport may not
// reuse HTTP/1.x "keep-alive" TCP connections if the Body is
// not read to completion and closed.
The same principal I think applies for the r *http.Request since both bodies are the same type.
I don't know if I am missing something here but if this is true, then we should drain the client body as well.
There was a problem hiding this comment.
So there are two things here, whether you need to drain and whether you need to close.
I was referring to manually closing the request body bit which you don't need to do.
The text about the response body does not apply to the request body. They might have the same type but that does not mean the same things happen to them in the lifecycle of an HTTP call.
When making a call as a client and reading the response, Go does not know when you are done with that so you need to manually close. When receiving a call as a server, the whole round trip ends when the handler function returns. At that point at latest, the request body can automatically be closed.
Regarding the draining I have also now read this multiple times that it is recommended to drain. I don't think we have to go into the details of this, stating that is is important to drain, maybe saying how to do that if you don't need the data and then providing some link should be good enough for now. Also I would consider this "about" content as it is not needed for the exercise as we don't have this edge case there that someone sends data that we don't want to read. (In real life, even in error cases you usually read the data to log the error message.)
There was a problem hiding this comment.
I also had a look at the exercises now and left my thoughts there as mentioned earlier.
As you can guess from the comments, there is still some work to be done here but you are on a good track. As we might have said before, if at any point you feel like you don't want to continue working on this, we can always merge this with "wip" status and create follow up issues to tackle the rest. Just let us know.
If you can manage to finish these two sets, I would probably set this so it awards 100 rep + the rep from the authoring because it was a huge effort and much more than one standard exercise + concept.
exercises/concept/weather-forecast-client/.docs/instructions.md
Outdated
Show resolved
Hide resolved
exercises/concept/weather-forecast-server/.docs/instructions.md
Outdated
Show resolved
Hide resolved
exercises/concept/weather-forecast-server/.docs/instructions.md
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,37 @@ | |||
| package weatherforecastserver | |||
There was a problem hiding this comment.
The general setup in this package should be similar to what I described for the client. You have a custom server struct (one field of which is the standard server) with a New constructor and then that struct should have a Handle method that takes a path and a Handler function as parameters and adds those to the multiplexer.
(Imagine you are building your own, very lightweight web framework. You can see a more elaborate example of this pattern here: https://github.com/ardanlabs/service/blob/08f7a8eb7b9c374becc38ded3261255cd10b2dca/foundation/web/web.go)
So the tasks for the student would be to set up the struct, constructor and Handle function (probably it makes sense to make this one task), then the next task is adding a GET route via the handle method and then the last task is adding the POST route via the handle method. You probably avoid issues by using different route names for the GET and POST route even though that is not realistic in a standard REST scenario.
There was a problem hiding this comment.
@junedev
To be honest I am having a bit of trouble completing this part.
So let me go through your comment step by step so I can make sure I understand every bit clearly:
-
The general setup in this package should be similar to what I described for the client. You have a custom server struct (one field of which is the standard server):
type WeatherServer struct{httpServer *http.Server, ...}
-
with a New constructor:
func NewWeatherServer() WeatherServer
-
and then that struct should have a Handle method that takes a path and a Handler function as parameters:
func (ws *WeatherServer) Handle(path string, handler http.Handler)
-
adds those to the multiplexer:
So for this part I have a little bit of problem. First of all, since we want to pass this as the Handler for the server, this process should be done before defining the server in the NewWeatherServer function. The solution that comes to my mind is to keep an array of handlers in the WeatherServer structure and use the Handle to add to this array, and use the array to create the mux during initializing of the server. The problem with this approach is that we cannot use Handle before creating a WeatherServer, unless we do not initializ the httpServer in NewWeatherServer. I have looked at the example but they use *httptreemux.ContextMux as their mux type so that is not applicable to this solution. Did I understand this currectly and does my solution make sense for it?
-
So the tasks for the student would be to set up the struct, constructor and Handle function (probably it makes sense to make this one task)
-
then the next task is adding a GET route via the handle method:
Which is basically to compose a handler to feed to Handle function;
-
and then the last task is adding the POST route via the handle method.
Same as 6.
I have looked for building web frameworks in Go, these are the best results that I came along, but none implemented things in the same manner:
https://go.dev/doc/articles/wiki/
https://semaphoreci.com/community/tutorials/building-go-web-applications-and-microservices-using-gin
https://blog.logrocket.com/creating-a-web-server-with-golang/
There was a problem hiding this comment.
I am not sure why you think our mux is so different from the one I provided in the example and I am not sure why you think we cannot add handlers later on.
- For the scope of the exercise, let us please do the normal way of simply reading the request in the handler function (or discarding it there). There is already so much too take in here for students, the middleware makes this harder to see through.
- As for the struct, that was a bit confusing. The important part is wrapping the mux but I see that it is still hard to construct an example that resembles real code here. Taking a step back, the important thing to learn here is how to write HTTP handler functions, how to get the data from the request etc. Passing this address to a server is rather specific to our setup here.
So what about changing it so the student basically only has to do line 16-26 (plus the part with the post route). We would ask the student to write a Setup function that accepts a mux (http.Handler) and adds a GET route. Then in task 2 we would ask the student to extend the Setup function with the post route. The test code would call Setup to get the completed mux and then set up the server by itself.
Even better plan:
- Task 1 is to write only the handler function for the GET route. It can easily be tested invidiually with the httptest package.
- Task 2 is to write only the handler funtion for the POST route.
- Task 3 is to write the Setup function that adds the above functions to a mux with the correct path as described above.
That would pretty much resemble what you do in a real app.
junedev
added
the
status/awaiting-contributor
Thank you for letting me know. To be honest, I love to keep keep working on this concept, but recently I have been really busy and even had problems getting access to the Internet. I understand it might be inconvenient to have a concept hanging this long, and I totally understand if you would like to merge it with "wip" status. But in both cases, I will work on it and try to complete it as soon as possible. |
|
In that case, feel free to chip away at your own pace. Just wanted to inform you about that possibility with the wip merge. |
…lained in this concept at the top
There was a problem hiding this comment.
Finally had time to look at your comments/questions.
In the light of this https://exercism.org/blog/freeing-our-maintainers, I would like to go back to the other plan of merging this as WIP soon-ish. So if you take another pass at this, try to get it to a state where CI is green (comment out things if necessary) and then I will merge it.
| w.Write([]byte("Hello World!")) | ||
| }) | ||
| return serveMux | ||
| } |
There was a problem hiding this comment.
I did not mean the handler function, I meant why is the example code not just
serveMux := http.NewServeMux()
serveMux := http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request){
w.Write([]byte("Hello World!"))
})
|
|
||
| ~~~~exercism/caution | ||
| Draining the request body before closing it: | ||
| Unlike the client that drains the body before closing it, in the server you need to manually drain the request body. A good way to do so is writing a middleware that closes the request after the server job is done (I have seen this pattern for the first time in [Adam Woodbeck's mux_test.go][awoodbeck-gnp-chapter09-mux_test.go]): |
There was a problem hiding this comment.
So there are two things here, whether you need to drain and whether you need to close.
I was referring to manually closing the request body bit which you don't need to do.
The text about the response body does not apply to the request body. They might have the same type but that does not mean the same things happen to them in the lifecycle of an HTTP call.
When making a call as a client and reading the response, Go does not know when you are done with that so you need to manually close. When receiving a call as a server, the whole round trip ends when the handler function returns. At that point at latest, the request body can automatically be closed.
Regarding the draining I have also now read this multiple times that it is recommended to drain. I don't think we have to go into the details of this, stating that is is important to drain, maybe saying how to do that if you don't need the data and then providing some link should be good enough for now. Also I would consider this "about" content as it is not needed for the exercise as we don't have this edge case there that someone sends data that we don't want to read. (In real life, even in error cases you usually read the data to log the error message.)
|
|
||
| func multiplexer() http.Handler{ | ||
| serveMux := http.NewServeMux() | ||
| serveMux := http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request){ |
There was a problem hiding this comment.
serveMux := appears twice here, that doesn't look right
| @@ -0,0 +1,37 @@ | |||
| package weatherforecastserver | |||
There was a problem hiding this comment.
I am not sure why you think our mux is so different from the one I provided in the example and I am not sure why you think we cannot add handlers later on.
- For the scope of the exercise, let us please do the normal way of simply reading the request in the handler function (or discarding it there). There is already so much too take in here for students, the middleware makes this harder to see through.
- As for the struct, that was a bit confusing. The important part is wrapping the mux but I see that it is still hard to construct an example that resembles real code here. Taking a step back, the important thing to learn here is how to write HTTP handler functions, how to get the data from the request etc. Passing this address to a server is rather specific to our setup here.
So what about changing it so the student basically only has to do line 16-26 (plus the part with the post route). We would ask the student to write a Setup function that accepts a mux (http.Handler) and adds a GET route. Then in task 2 we would ask the student to extend the Setup function with the post route. The test code would call Setup to get the completed mux and then set up the server by itself.
Even better plan:
- Task 1 is to write only the handler function for the GET route. It can easily be tested invidiually with the httptest package.
- Task 2 is to write only the handler funtion for the POST route.
- Task 3 is to write the Setup function that adds the above functions to a mux with the correct path as described above.
That would pretty much resemble what you do in a real app.
Hi!
I have written the client section for issue #2242 . I am creating a PR early and set it as a draft because I would like to hear ideas on various sections as I write each section.
I would be really happy if you could read it and let me know if any ideas or points come to your mind.