Skip to content
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

Jump to bottom

x/website/_content/doc/tutorial: "Developing a RESTful API with Go and Gin" has unmentioned race condition #51097

Open
vearutop opened this issue Feb 9, 2022 · 3 comments
Open

x/website/_content/doc/tutorial: "Developing a RESTful API with Go and Gin" has unmentioned race condition #51097

vearutop opened this issue Feb 9, 2022 · 3 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unreleased

Comments

@vearutop
Copy link
Contributor

vearutop commented Feb 9, 2022

This tutorial https://go.dev/doc/tutorial/web-service-gin uses package variable album to read and write from http handlers.
However there is no synchronization applied to avoid race condition.

// postAlbums adds an album from JSON received in the request body.
func postAlbums(c *gin.Context) {
    var newAlbum album

    // Call BindJSON to bind the received JSON to
    // newAlbum.
    if err := c.BindJSON(&newAlbum); err != nil {
        return
    }

    // Add the new album to the slice.
    albums = append(albums, newAlbum)                       // <-- DATA RACE.
    c.IndentedJSON(http.StatusCreated, newAlbum)
}

If this is intentional to keep the tutorial simple I think it makes sense to warn the reader about code limitations, because high level of concurrency is natural for http servers.

Or maybe it would be even better to avoid race with a different design.

Thanks.
@gopherbot gopherbot added this to the Unreleased milestone Feb 9, 2022
@mvdan
Copy link
Member

mvdan commented Feb 9, 2022

This was brought up by @benhoyt in https://groups.google.com/g/golang-dev/c/kC7YZsHTw4Y/m/u0_d9lQoAwAJ; it seems like the response from @rsc was that they want to leave the tutorial as-is. I personally tend to agree with Ben, in that I don't particularly like that the tutorial is racy and that it arbitrarily chooses gin without any reasoning.

@giorgiozoppi
Copy link

giorgiozoppi commented Feb 9, 2022 •
edited

A better approach will be just using just standard HTTP or gorilla/mux that it has a wider usage in the community, Agreed about the data race. We mandate a correct approch in learning.

@cherrymui cherrymui added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 11, 2022
@cherrymui
Copy link
Member

This may be working as intended. But leave to @rsc @dmitshur to decide. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@vearutop @mvdan @gopherbot @cherrymui @giorgiozoppi