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

Discussion: Is it possible to add a GODEBUG option to disable the "cooperative goroutine preemption" #39327

Closed
linguohua opened this issue May 30, 2020 · 7 comments

Comments

@linguohua
Copy link

@linguohua linguohua commented May 30, 2020

Discussion: Is it possible for golang runtime to add a GODEBUG option to disable the "cooperative goroutine preemption"

Allow me to introduce our app logic code organization method:
We use a single-threaded multi-process approach, similar to nodejs and nginx, the logic code run in a single-threaded environment, and we deploy multiple instances to ultilize the multi-cores of the server.
The advantage of running logic code in a single-threaded environment is that we do not need mutex to protect most public variables, only need to handle a few cases caused by async IO (such as access to redis).

However, due to the "cooperative goroutine preemption" introduced by golang 1.1x: when a goroutine runs for more than 10ms, it will be set to preemptable by sysmon, and it will be preempted when it enters the runtime.newstack function(caused by any function call).
Therefore, we now need mutex to protect all public variables access (e.g.: a map), but this does increase our development burden, because we need to consider where to lock or unlock.

Therefore, I hope to have a GODEBUG option, similar to the "asyncpreemptoff" option of "signal-based asynchronous goroutine preemption", thus we can disable the "cooperative goroutine preemption" when we need to.

Anticipate your suggestions, thanks so much.

runtime code relative:

} else if pd.schedwhen+forcePreemptNS <= now {

@randall77
Copy link
Contributor

@randall77 randall77 commented May 31, 2020

I don't understand the problem you're trying to solve.

You're writing a single-threaded program. Don't use the go statement. Then you won't need locks on your global variables. It's irrelevant whether, or how, the runtime interrupts your goroutine occasionally to get its own work done.

In any case, this issue tracker isn't the right place for this kind of discussion. Please see https://golang.org/wiki/Questions for good places to ask.

@randall77 randall77 closed this May 31, 2020
@linguohua
Copy link
Author

@linguohua linguohua commented May 31, 2020

@randall77 Thanks for your reply, and sorry for that I did not make my problem clear.
For example, I have a simple http server, this is my main function:

func main() {
    runtime.MAXGOPROC(1); // use only one thread to run all goroutines
    ...... // start http server
}

And, each client request will call into this function:

func simpleHandler(r*Request) {
    ...... // do long time cost job, consume almost 10ms time
   aMap[key] = value // access to a global map instance named aMap
}

as you sugguest, I do not use "go" to create new goroutine, but the HTTP runtime does: every new client request correspond to a new goroutine.
because long time cost job has cost almost 10ms, and when it run into "aMap[key]=value", the goroutine will have great chance to be preemted, and then another goroutine read the map, it will panic:

throw("concurrent map read and map write")

If I can disable the "cooperative goroutine preemption", then everything goes right.

@davecheney
Copy link
Contributor

@davecheney davecheney commented May 31, 2020

Regardless of the GOMAXPROC setting your program has a logical data race, and is considered by the go specification to be undefined. We're not going to change the language to support this use case but maybe you could explain the underlying problem that has lead you to use a shared map, there may be another way to solve your underlying problem without changing the language.

@linguohua
Copy link
Author

@linguohua linguohua commented May 31, 2020

@davecheney Thanks for your reply.
I recognize that I can use mutex, but the most important thing to me, is how can we get rid of the missing lock/unlock of mutex.? Maybe someday, somebody goes wrong, and then a thread safety(or concurrent safety) bug come, unfortunately, this type of bug show up randomly, and is difficult to reproduce and fix.

If golang has the facility just like rust does, utilize the compiler to force things to be thread safety, then mutex is free to use(without the consideraton of performance). But for now, golang need the programer to ensure thread safety, that make my hair curl.

I can set GODEBUG=asyncpreeptoff=1 to disable async preemption, and runtime.GOMAXPROC(1) to use only one thread to run all goroutines, then, if we can disable the '10ms coorperative preemption' too, we need not to use mutex most of time.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 31, 2020

@linguohua Have you seen https://blog.golang.org/race-detector and https://golang.org/doc/articles/race_detector.html ? The race detector can help detect a missing lock/unlock of a mutex.

@davecheney
Copy link
Contributor

@davecheney davecheney commented May 31, 2020

Thank you for explaining your use case.

Generally the practice to ensure lock invariants are followed is to place the type under the mutex and the mutex itself into a struct and drive access to those values through methods on that struct.

Alternatively, grant the ownership of the thing under the lock to a single goroutine and mediate access to that state via messages over a channel.

Lastly, pervasive use of the race detector in test and canary deploys is recommended to local data races.

@linguohua
Copy link
Author

@linguohua linguohua commented Jun 1, 2020

@ianlancetaylor @davecheney Thank you both.
I will follow your advices to refactor our code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.