golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.41k stars 17.71k forks source link

proposal: iter: relax Pull to not panic because of thread locking state #67694

Open eliasnaur opened 6 months ago

eliasnaur commented 6 months ago

Proposal Details

iter.Pull snapshots the thread lock count and locked thread, if any. Calling next, stop or yield with a different snapshot panics. This proposal is about lifting that restriction.

Adapted from my longer explanation at https://github.com/golang/go/issues/65889#issuecomment-2059844896, the proposal is to replace the panic behaviour with cooperative scheduling:

Let the locked thread (if any) and its lock count follow goroutine switches between caller and iterator, making next, stop and yield act as cooperative scheduling points. That is, the thread (and lock count) entering next, stop or yield is the same thread (and lock count) that resumes execution from the return of its counterpart. For example, calling next from (locked) thread T will see yield return (locked) on thread T and vice versa.

Notable consequences:

Other than making iter.Pull more widely useful by eliminating panics, this proposal elegantly fixes https://github.com/golang/go/issues/64755 and https://github.com/golang/go/issues/64777 by thread stealing. For example:

// Package mainthreadapi implements a platform API that require the main thread.
package mainthreadapi

import "iter"
import "C"

func init() {
  // Lock the main thread.
  runtime.LockOSThread()
  next, _ := iter.Pull(func(yield func(struct{}) bool) {
       go yield(struct{}{}) // Pass a different thread to the caller.
       C.native_event_loop_that_require_the_main_thread_and_never_returns()
  })
  next() // Pass the main thread to the iterator
  // Continues on a different thread.
}

// API functions follow.
hajimehoshi commented 6 months ago

Just out of curiosity, will the next init be called on the main thread, or a different thread? If init is called on a different thread, wouldn't this break some existing libraries that assume init must be called on the main thread?

eliasnaur commented 6 months ago

The next init will run on a different thread. I assume few packages require the main thread during initialization, and packages that do should add a check for the main thread.

Some packages call LockOSThread during init to lock the main thread to the main goroutine, but they wouldn't notice the thread is different. The package API meant to be called from the main goroutine need a main thread check anyway to guard against the user calling the package from a different goroutine.

hajimehoshi commented 6 months ago

I thought some OS APIs must be called from the main thread (= the thread that is for init/main), but I might misunderstand. I'll look for an example.

Another thing I came up with is the opposite pattern:

  1. Call runtime.LockOSThread in a library A
  2. Call iter.Pull in a library B
  3. The main function runs on a thread that is different from the locked thread in A

For example, if an OpenGL context is initialized for the current thread at 1, and the main function calls GL functions, the application no longer works due to 2.

eliasnaur commented 6 months ago

I thought some OS APIs must be called from the main thread (= the thread that is for init/main), but I might misunderstand. I'll look for an example.

Absolutely, and in your example the "next init" that expect the main thread would fail, either immediately because of an is-main-thread check or because calls to its main-thread native API fails.

My point is that it's already the case the two or more packages all requiring the main thread needs some kind of coordination to work together. The iter.Pull pattern in my proposal merely allows a package a way to hide its main thread requirement in its public API.

Another thing I came up with is the opposite pattern:

1. Call `runtime.LockOSThread` in a library A

2. Call `iter.Pull` in a library B

3. The main function runs on a thread that is different from the locked thread in A

I consider it unlikely, or at least bad design, to actually use the main thread during initialization. LockOSThread during initialization is only to force the Go runtime to lock the main thread to the main function/goroutine.

For example, if an OpenGL context is initialized for the current thread at 1, and the main function calls GL functions, the application no longer works due to 2.

In your stated example, yes. However, a well-designed OpenGL package would not call LockOSThread until creation of an OpenGL context, because OpenGL only require some locked thread, not necessarily the main thread.

thediveo commented 6 months ago

locking the main thread in init is also needed to avoid tripping Linux kernel namespace leak checks, when the initial thread get only later locked and then tainted. Without initial locking the initial thread cannot be destroyed, but permanently wedged, to use Go's runtime terminology. Locking the initial thread ensures that when later needing locking for namespace switches will happen on non-initial go routines and this on threads than can be thrown away, the reason being that this happens, for instance as part of an http handler goroutine.

mknyszek commented 6 months ago

What makes me personally uncomfortable with the semantics you suggest is the goroutine executing init, without ever calling runtime.UnlockOSThread, has managed to lose its OS thread lock. This is very unintuitive and surprising, IMO.

I also don't see how that's preferable to something like RunOnMainThread (#64777). If multiple packages try to call something like C.native_event_loop_that_require_the_main_thread_and_never_returns() in this proposal, then I assume that the API call will fail (or hang, or crash in some weird way due to a race, or whatever it does when it's not actually called from the main thread). In the RunOnMainThread proposal (at least, without any additional tweaks), all but one of these packages' init functions will hang, but you don't actually violate any invariants.