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

doc: Channels example in Effective Go leaks goroutines #33804

Open
jakekdodd opened this issue Aug 23, 2019 · 7 comments

Comments

@jakekdodd
Copy link

commented Aug 23, 2019

The final example in Effective Go's Channels section leaks goroutines. Here's a solution that preserves the original semantics of Serve, and is closest to a similar example presented in Tour:

func Serve(clientRequests chan *Request, quit chan bool) {
    reqs := make(chan *Request) // Dispatch queue for handlers.
    defer close(reqs) // Ensures handler goroutines complete.

    // Start handlers
    for i := 0; i < MaxOutstanding; i++ {
        go handle(reqs)
    }

    for {
        select {
        case req := <-clientRequests:
            reqs <-req
        case <-quit: // Wait to be told to exit.
            return
        }
    }
}
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

@as

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

I dont understand how the original example leaks goroutines. The handler goroutines (which are long-living and bounded) will go away when the request channel is closed.

@jakekdodd

This comment has been minimized.

Copy link
Author

commented Aug 23, 2019

I realize that, but this is the mechanism that leads to goroutine leaks. It's a subtle point not mentioned in Effective Go or in Tour and, as of now, the docs actually guide users towards buggy usage of channels. Leaked goroutines are one of the most common, if not the most common, concurrency-related bugs in Go programs. If you put yourself in the shoes of a programmer new to Go, the current snippet strongly suggests that the way to exit Serve is to signal through the quit channel—which on its own leads to a leak.

I thought about suggesting we put a doc comment in the snippet explaining that the clientRequests channel needs to be closed in addition to signaling an exit, but my gut says that will confuse people more than clarify. They'll be thinking "why do we need the quit channel in the first place?"; or, "why did Tour say I only need to close a channel when using a range loop?"

As it stands, this snippet is suggesting it's perfectly fine for an exported function to hold goroutines open indefinitely, even after signaling it to exit through a channel...I can't imagine a good argument for recommending that practice.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

See #28782. I have resisted changes like this in the past not because your reasoning is wrong, but because the whole document needs to be replaced.

If we do anything for your issue, it should be no more than a clarifying sentence.

@jakekdodd

This comment has been minimized.

Copy link
Author

commented Aug 23, 2019

That’s fair—I think a single sentence is sufficient, something like:

“In the snippet below, the handler goroutines will remain in memory and blocked on clientRequests (even after Serve exits) until the channel is closed.”

@robpike robpike self-assigned this Aug 23, 2019

@as

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

They'll be thinking "why do we need the quit channel in the first place?"; or, "why did Tour say I only need to close a channel when using a range loop?"

Closing a channel tells consumers that the producer has no further data to send, but buffered data may still be marinating in that channel. As such, the consumer won't be made-aware of the close until the entire buffet is eaten.

The quit channel is an interrupt that should take priority over the already-buffered items. It would make more sense for the handlers to know about the quit channel than have complex re-serialization logic that merely stops sending items to the handlers.

@jakekdodd

This comment has been minimized.

Copy link
Author

commented Aug 26, 2019

@as sure—as I said, I tried to pick a solution that didn't rearrange the program too dramatically. There was a choice between transferring elements from clientRequests to a handler dispatch queue, or changing the signature of the handlers to require a quit channel. The latter also requires creating MaxOutstanding quit channels and broadcasting to them from within Serve, right? I thought my choice is the simpler solution and that's why I chose it; but aside from hoping the docs don't encourage leaking goroutines, I don't have any opinions here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.