Skip to content
This repository was archived by the owner on Jul 19, 2019. It is now read-only.

Update Go server #30

Closed
wants to merge 2 commits into from
Closed

Update Go server #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99086bd
Update editorconfig for go
zpao Feb 12, 2015
3fbd7c4
Update Go server
zpao Feb 12, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,8 @@ insert_final_newline = true
trim_trailing_whitespace = true

[*.{js,rb}]
indent_size=2
indent_size = 2

[*.go]
indent_size = 8
indent_style = tab
12 changes: 9 additions & 3 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,17 @@ func handleComments(w http.ResponseWriter, r *http.Request) {
return
}

// Write out the json response
commentsEncoder := json.NewEncoder(cFile)
commentsEncoder.Encode(comments)
// Pretty print the JSON, write to file
var encodedComments, _ = json.MarshalIndent(comments, "", " ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 problems here:

  1. The use of var encodedComment... vs encodedComments :=
  2. ignoring the error is bad.
  3. This causes the entirety of the encoded data to be stored in ram in encodedComments. The previous solution handled it via streaming readers and writers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What's the difference? (I totally missed that)
  2. I usually agree. We've been pretty lazy in our other servers but I could handle it.
  3. It gets GCed right? It's not a huge deal but I wanted the file pretty-printed (since we talk about manually editing the file in the tutorial). I didn't see how to use MarshalIndent with Encode. It also felt a bit silly to write the file and then immediately read it (my first pass was to just io.Copy(w, cFile) like in the GET handler).

Like I said, I haven't written Go. If we could get this pedantically idiomatic and keep the JSON formatting that would be awesome! Either way, nobody should be taking these servers and putting them into production ;) , so it's not the end of the world if we don't get it perfect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't think you can actually do multiple assignment with var, only :=, but you're throwing away the err so it maybe doesn't matter in this case? change it to be...
encodedComments, err := json.MarshalIndent(comments, "", "    ")
if err != nil {
    http.Error(w, fmt.Sprintf("Unable to marshal comments to json: %s", err), http.StatusInternalServerError)
    return
}
  1. See above
  2. So yes, it would be GC'd. But, it's just generally bad practice to use the non streaming version. Since this is an example I'd rather be exemplifying the "right" (some would probably argue it's not), vs the most convenient.

FWIW: The issue with using {Un,}Marhsal is that they read in the entire source and return byte buffers. So, pretend the json file is 50MB in size. So for each post request that would use the Marshal/Unmarshal pair (vs the encoders) the file would need to be read into a []byte, which would be ~50MB. Then you need to UnMarshal that into the []comment, which would be another data structure of ~50 MB modulo JSON, and then another 50MB+ []byte get's created when you MarshalIndent the updated comment back to JSON. So memory usage is ~150MB doing it this way. I haven't run any specific tests, but the pure Encoder/Decoder version of this will use ~50MB total, most of it because we realize the fully constructed []comment.

However this is an example, and ... AFAICT the other examples are doing it by just reading everything into ram / constructing arbitrary sized objects in ram and then the reverse to write data back out. So maybe I'm being too picky about the example?

I'd be happy to submit a pull request that switches everything to using UnMarshal/MarshalIndent instead if you think it's best.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you? That would be awesome. I got my fill of writing Go for the time being :D

The servers implementations are by far the least important part of the whole thing here. The variety is intended to make sure anybody can run something without having to muck around too much with installing different thing (eg, shipping just a Go server would be bad). Keeping the behavior consistent across these servers is important to me though because it makes it easier to help people. No saying "oh you're using the Go server and it does things slightly differently... normally you wouldn't have to parse a single long line of JSON in your head".

So if you wouldn't mind doing that, I'd appreciate it. If you wanted to add comments saying that "normally you wouldn't do this, you would just do X, Y, Z" instead, that's totally fine.

cFile.WriteAt(encodedComments, 0)

// send the encoded comments to the response
w.Header().Set("Content-Type", "application/json")
w.Write(encodedComments)

case "GET":
// stream the contents of the file to the response
w.Header().Set("Content-Type", "application/json")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, sorry I missed that.

io.Copy(w, cFile)

default:
Expand All @@ -81,5 +86,6 @@ func handleComments(w http.ResponseWriter, r *http.Request) {
func main() {
http.HandleFunc("/comments.json", handleComments)
http.Handle("/", http.FileServer(http.Dir("./public")))
log.Println("Server started: http://localhost:3000")
log.Fatal(http.ListenAndServe(":3000", nil))
}