runtime: terminate locked OS thread if its goroutine exits #20395

Closed
rgooch opened this Issue May 17, 2017 · 40 comments

Comments

Projects
None yet
10 participants
@rgooch

rgooch commented May 17, 2017

I propose a new runtime.TaintOSThread function. This would mark the current thread that the calling goroutine is running on as being "tainted": not safe for use by other goroutines. When the goroutine exits or the goroutine is unpinned from the (previously pinned) OS thread, the thread is killed.

This is needed when goroutines are pinned to OS threads and the state of the thread is modified in way that is unsafe for other goroutines, such as unsharing and then modifying one of the Linux namespaces. Without this change, it is impossible to clean up OS threads: the pinning goroutines and OS threads need to stay until the programme exits, leading to unfixable goroutine and OS thread leaks.

The expected calling pattern is:

runtime.LockOSThread()
runtime.TaintOSThread()
syscall.Unshare(syscall.CLONE_NEWNS) // Example mutation of OS thread state.

@gopherbot gopherbot added this to the Proposal milestone May 17, 2017

@gopherbot gopherbot added the Proposal label May 17, 2017

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz May 17, 2017

Member

Could you instead just LockOSThread + Unshare + then kill that thread yourself?

Member

bradfitz commented May 17, 2017

Could you instead just LockOSThread + Unshare + then kill that thread yourself?

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch May 17, 2017

How can I kill the thread safely? I don't see a function in the runtime package to do this. If I can safely kill the thread on which a goroutine is running, then that would work too, I just didn't think that would be as palatable as marking a thread as tainted.

There's actually no need to unshare (and that wouldn't help anyway, since the new namespace inherits from its ancestor).

rgooch commented May 17, 2017

How can I kill the thread safely? I don't see a function in the runtime package to do this. If I can safely kill the thread on which a goroutine is running, then that would work too, I just didn't think that would be as palatable as marking a thread as tainted.

There's actually no need to unshare (and that wouldn't help anyway, since the new namespace inherits from its ancestor).

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz May 17, 2017

Member

syscall.Syscall(syscall.SYS_EXIT, 0, 0, 0)? Haven't tried it, though.

Member

bradfitz commented May 17, 2017

syscall.Syscall(syscall.SYS_EXIT, 0, 0, 0)? Haven't tried it, though.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz May 17, 2017

Member

Works for me:

bradfitz@gdev:~$ cat exit.go
package main

import (
        "runtime"
        "syscall"
        "time"
)       

func main() {
        runtime.LockOSThread()
        println("Hi from ", syscall.Gettid())
        go func() {
                runtime.LockOSThread()
                println("Hi from ", syscall.Gettid())
                syscall.Syscall(syscall.SYS_EXIT, 0, 0, 0)
        }()     
        time.Sleep(500 * time.Millisecond)
        println("Bye from ", syscall.Gettid())
}    
bradfitz@gdev:~$ go run exit.go
Hi from  6876
Hi from  6878
Bye from  6876
Member

bradfitz commented May 17, 2017

Works for me:

bradfitz@gdev:~$ cat exit.go
package main

import (
        "runtime"
        "syscall"
        "time"
)       

func main() {
        runtime.LockOSThread()
        println("Hi from ", syscall.Gettid())
        go func() {
                runtime.LockOSThread()
                println("Hi from ", syscall.Gettid())
                syscall.Syscall(syscall.SYS_EXIT, 0, 0, 0)
        }()     
        time.Sleep(500 * time.Millisecond)
        println("Bye from ", syscall.Gettid())
}    
bradfitz@gdev:~$ go run exit.go
Hi from  6876
Hi from  6878
Bye from  6876
@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz May 17, 2017

Member

Or, better: (showing it actually went away)

package main

import (
        "fmt"
        "os"
        "runtime"
        "strconv"
        "syscall"
        "time"
)

func main() {
        runtime.LockOSThread()
        fmt.Println("Hi from ", syscall.Gettid())
        other := make(chan int, 1)
        go func() {
                runtime.LockOSThread()
                tid := syscall.Gettid()
                other <- tid
                fmt.Println("Hi from ", tid)
                fi, err := os.Stat("/proc/" + strconv.Itoa(tid))
                fmt.Printf("proc %d: %v, %v\n", tid, fi, err)
                syscall.Syscall(syscall.SYS_EXIT, 0, 0, 0)
        }()
        time.Sleep(500 * time.Millisecond)
        tid := <-other
        fi, err := os.Stat("/proc/" + strconv.Itoa(tid))
        fmt.Printf("proc %d: %v, %v\n", tid, fi, err)
        fmt.Println("Bye from ", syscall.Gettid())
}
Hi from  7277
Hi from  7279
proc 7279: &{7279 0 2147484013 {63630648429 114580211 0x550860} 0xc20804e000}, <nil>
proc 7279: <nil>, stat /proc/7279: no such file or directory
Bye from  7277
Member

bradfitz commented May 17, 2017

Or, better: (showing it actually went away)

package main

import (
        "fmt"
        "os"
        "runtime"
        "strconv"
        "syscall"
        "time"
)

func main() {
        runtime.LockOSThread()
        fmt.Println("Hi from ", syscall.Gettid())
        other := make(chan int, 1)
        go func() {
                runtime.LockOSThread()
                tid := syscall.Gettid()
                other <- tid
                fmt.Println("Hi from ", tid)
                fi, err := os.Stat("/proc/" + strconv.Itoa(tid))
                fmt.Printf("proc %d: %v, %v\n", tid, fi, err)
                syscall.Syscall(syscall.SYS_EXIT, 0, 0, 0)
        }()
        time.Sleep(500 * time.Millisecond)
        tid := <-other
        fi, err := os.Stat("/proc/" + strconv.Itoa(tid))
        fmt.Printf("proc %d: %v, %v\n", tid, fi, err)
        fmt.Println("Bye from ", syscall.Gettid())
}
Hi from  7277
Hi from  7279
proc 7279: &{7279 0 2147484013 {63630648429 114580211 0x550860} 0xc20804e000}, <nil>
proc 7279: <nil>, stat /proc/7279: no such file or directory
Bye from  7277
@randall77

This comment has been minimized.

Show comment
Hide comment
@randall77

randall77 May 17, 2017

Contributor

I worry that the runtime would be upset at an OS thread permanently vanishing. The runtime may want notification to ensure that anything the m is holding is freed first.
Maybe the existing enterSysCall already does everything that might matter?
At the very least, the allm list will grow without bound.

Contributor

randall77 commented May 17, 2017

I worry that the runtime would be upset at an OS thread permanently vanishing. The runtime may want notification to ensure that anything the m is holding is freed first.
Maybe the existing enterSysCall already does everything that might matter?
At the very least, the allm list will grow without bound.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch May 17, 2017

Yes, that was one of my worries too. Can we get some kind of guidance on whether it's considered safe to kill a thread? What happens to the goroutine that's running on it?

rgooch commented May 17, 2017

Yes, that was one of my worries too. Can we get some kind of guidance on whether it's considered safe to kill a thread? What happens to the goroutine that's running on it?

@randall77

This comment has been minimized.

Show comment
Hide comment
Contributor

randall77 commented May 17, 2017

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor May 17, 2017

Contributor

I do think we need to let the runtime know that the thread can not be used. The simple approach is

runtime.LockOSThread()
select{}

That will ensure that nothing else uses the thread, but it won't actually kill the thread. It would be an acceptable approach if you only need to do this a few times, but of course would be unsatisfactory if you need to do it continually.

Another approach would be to go through C. Have C code create a new thread, which could then call into Go. That gives you a thread that the Go runtime won't use itself, so you can simply call pthread_exit when done. Of course this is not ideal if you otherwise have a pure Go program.

Contributor

ianlancetaylor commented May 17, 2017

I do think we need to let the runtime know that the thread can not be used. The simple approach is

runtime.LockOSThread()
select{}

That will ensure that nothing else uses the thread, but it won't actually kill the thread. It would be an acceptable approach if you only need to do this a few times, but of course would be unsatisfactory if you need to do it continually.

Another approach would be to go through C. Have C code create a new thread, which could then call into Go. That gives you a thread that the Go runtime won't use itself, so you can simply call pthread_exit when done. Of course this is not ideal if you otherwise have a pure Go program.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch May 17, 2017

All my Go code so far is pure Go. I was hoping to keep it that way.

It seems to me that we need some way to cleanly kill a thread. The hack shown above is, well, a hack :-)

rgooch commented May 17, 2017

All my Go code so far is pure Go. I was hoping to keep it that way.

It seems to me that we need some way to cleanly kill a thread. The hack shown above is, well, a hack :-)

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor May 17, 2017

Contributor

Is runtime.ThreadExit sufficient for your purposes? I'm thinking of a function that would simply cause the thread to exit and not return. The goroutine could continue running on a different thread.

I'll note that I appreciate that you need this, but it seems awfully special purpose.

Contributor

ianlancetaylor commented May 17, 2017

Is runtime.ThreadExit sufficient for your purposes? I'm thinking of a function that would simply cause the thread to exit and not return. The goroutine could continue running on a different thread.

I'll note that I appreciate that you need this, but it seems awfully special purpose.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch May 17, 2017

Yes, runtime.ThreadExit would work fine. I'm not special, I'm just ahead of the curve :-)

rgooch commented May 17, 2017

Yes, runtime.ThreadExit would work fine. I'm not special, I'm just ahead of the curve :-)

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz May 17, 2017

Member

@ianlancetaylor, or instead of introducing new API: just consider all threads "tainted" if they ever used LockOSThread and when the goroutine exits (implicitly or via the existing runtime.Goexit()), then kill the thread.

Member

bradfitz commented May 17, 2017

@ianlancetaylor, or instead of introducing new API: just consider all threads "tainted" if they ever used LockOSThread and when the goroutine exits (implicitly or via the existing runtime.Goexit()), then kill the thread.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch May 17, 2017

That works for me too.

rgooch commented May 17, 2017

That works for me too.

@rasky

This comment has been minimized.

Show comment
Hide comment
@rasky

rasky May 18, 2017

Contributor

Or maybe consider tainted all locked threads whose goroutine exits. If a goroutine calls Unlock before exiting, the thread is probably safe for reuse.

Contributor

rasky commented May 18, 2017

Or maybe consider tainted all locked threads whose goroutine exits. If a goroutine calls Unlock before exiting, the thread is probably safe for reuse.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch May 18, 2017

Yes, that works too. Which of these proposals is the most palatable?

rgooch commented May 18, 2017

Yes, that works too. Which of these proposals is the most palatable?

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements May 18, 2017

Member

I rather like @rasky's suggestion. If a goroutine exits without unlocking its thread, then you could think of that thread as still being locked, but with nothing it can ever do again, so you might as well kill it.

My only concern would be existing code that exits without unlocking implicitly unlocks the thread right now, so this change could cause such code to cycle through new OS threads, which could be a performance problem. But this all seems sufficiently obscure (and the fix for this performance problem is trivial) that I'm not too concerned.

Member

aclements commented May 18, 2017

I rather like @rasky's suggestion. If a goroutine exits without unlocking its thread, then you could think of that thread as still being locked, but with nothing it can ever do again, so you might as well kill it.

My only concern would be existing code that exits without unlocking implicitly unlocks the thread right now, so this change could cause such code to cycle through new OS threads, which could be a performance problem. But this all seems sufficiently obscure (and the fix for this performance problem is trivial) that I'm not too concerned.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch May 18, 2017

Indeed. Can we can get an approval from the core team for the proposal that @rasky made?

rgooch commented May 18, 2017

Indeed. Can we can get an approval from the core team for the proposal that @rasky made?

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz May 18, 2017

Member

@rgooch, we do proposal reviews roughly weekly, on Mondays. Sometimes we don't get through all open proposals and things wait another week.

Member

bradfitz commented May 18, 2017

@rgooch, we do proposal reviews roughly weekly, on Mondays. Sometimes we don't get through all open proposals and things wait another week.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch May 23, 2017

Did this get into the review this week?

rgooch commented May 23, 2017

Did this get into the review this week?

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz May 23, 2017

Member

Nope. We spent this week on NeedsDecision bugs (#20192).

When this gets reviewed, updates will be posted here.

Member

bradfitz commented May 23, 2017

Nope. We spent this week on NeedsDecision bugs (#20192).

When this gets reviewed, updates will be posted here.

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements May 23, 2017

Member

@rsc, @dr2chase, @ianlancetaylor and myself are all good with @rasky's suggestion. I haven't figured out how difficult it is to implement yet (it's certainly too late for Go 1.9).

Member

aclements commented May 23, 2017

@rsc, @dr2chase, @ianlancetaylor and myself are all good with @rasky's suggestion. I haven't figured out how difficult it is to implement yet (it's certainly too late for Go 1.9).

@aclements aclements self-assigned this May 23, 2017

@rsc rsc changed the title from proposal: add runtime.TaintOSThread function to runtime: terminate locked OS thread if its goroutine exits May 23, 2017

@rsc rsc modified the milestones: Go1.10, Proposal May 23, 2017

@gopherbot

This comment has been minimized.

Show comment
Hide comment

CL https://golang.org/cl/46038 mentions this issue.

@gopherbot

This comment has been minimized.

Show comment
Hide comment

CL https://golang.org/cl/46037 mentions this issue.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch Jun 17, 2017

Well, this is nice to see. Thank you. So, it looks like this will be available in go1.10 but not go1.9?

How can one add a compile assert so that a minimum go version is required to compile? Once this is available, I can relax certain restrictions in my code, but I need to be sure I have this fix otherwise it will be unsafe.

rgooch commented Jun 17, 2017

Well, this is nice to see. Thank you. So, it looks like this will be available in go1.10 but not go1.9?

How can one add a compile assert so that a minimum go version is required to compile? Once this is available, I can relax certain restrictions in my code, but I need to be sure I have this fix otherwise it will be unsafe.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Jun 17, 2017

Contributor

Use build tags, as in

// +build go1.10

to only compile a file for Go 1.10.

See "Build Constraints" in https://golang.org/pkg/go/build/ .

Contributor

ianlancetaylor commented Jun 17, 2017

Use build tags, as in

// +build go1.10

to only compile a file for Go 1.10.

See "Build Constraints" in https://golang.org/pkg/go/build/ .

@benschumacher

This comment has been minimized.

Show comment
Hide comment
@benschumacher

benschumacher Jul 28, 2017

Maybe an aside to this behavior change, but is there a definition of what a "clean" thread would be, w.r.t. to the current state. The premise here is that the developer will be responsible for returning a locked thread to some reusable state, but it's clear what this means in a practical sense.

Perhaps this is left to the end user, but it'd be good to have guidelines in documentation re: the basic assumptions in this implicit behavior.

Maybe an aside to this behavior change, but is there a definition of what a "clean" thread would be, w.r.t. to the current state. The premise here is that the developer will be responsible for returning a locked thread to some reusable state, but it's clear what this means in a practical sense.

Perhaps this is left to the end user, but it'd be good to have guidelines in documentation re: the basic assumptions in this implicit behavior.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch Jul 28, 2017

It's not possible to guarantee that a thread is returned to a previous state. For example, when unsharing a namespace, you may not be able to rejoin the original namespace. Thus, I think the safe approach is "once tainted, forever tainted. Kill it. Kill it with fire. Nuke it from orbit"

rgooch commented Jul 28, 2017

It's not possible to guarantee that a thread is returned to a previous state. For example, when unsharing a namespace, you may not be able to rejoin the original namespace. Thus, I think the safe approach is "once tainted, forever tainted. Kill it. Kill it with fire. Nuke it from orbit"

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Jul 28, 2017

Member

Maybe an aside to this behavior change, but is there a definition of what a "clean" thread would be, w.r.t. to the current state.

Really any change to thread state makes it unclean. That could mean namespace, priority, affinity, values in TLS, and probably at least a dozen other things that vary from OS to OS. If it's in any way distinguishable from any other thread in your process, it's probably not safe to return it to the thread pool. I'm not sure how to give useful guidelines on this, though I'm open to suggestions.

Member

aclements commented Jul 28, 2017

Maybe an aside to this behavior change, but is there a definition of what a "clean" thread would be, w.r.t. to the current state.

Really any change to thread state makes it unclean. That could mean namespace, priority, affinity, values in TLS, and probably at least a dozen other things that vary from OS to OS. If it's in any way distinguishable from any other thread in your process, it's probably not safe to return it to the thread pool. I'm not sure how to give useful guidelines on this, though I'm open to suggestions.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch Jul 28, 2017

Why even worry about this? The simple and safe thing to do is kill the thread. Creating a new thread is cheap enough. I doubt that threads are being tainted at a high rate, so the cost of creating a new thread is insignificant in this context.

rgooch commented Jul 28, 2017

Why even worry about this? The simple and safe thing to do is kill the thread. Creating a new thread is cheap enough. I doubt that threads are being tainted at a high rate, so the cost of creating a new thread is insignificant in this context.

@benschumacher

This comment has been minimized.

Show comment
Hide comment
@benschumacher

benschumacher Jul 29, 2017

Why even worry about this? The simple and safe thing to do is kill the thread. Creating a new thread is cheap enough. I doubt that threads are being tainted at a high rate, so the cost of creating a new thread is insignificant in this context.

Sure. To be clear, I'm not suggesting that user's are encouraged to try to reset thread state. I agree that tracking / observing all potential those changes is highly complex and dependent on operating environment. The alternative of just leading thread be reaped / recreate is almost certainly more efficient from runtime and development perspective.

I'm not sure how to give useful guidelines on this, though I'm open to suggestions.

I suppose the documentation could be a note on LockOSThread:

A goroutine's thread context can be isolated with LockOSThread to allow modifications to that thread's system-level context, such as namespace, uid, gid, etc. Leaving the OS Thread locked at the completion of the goroutine indicates that the thread is polluted and should not be reused by other goroutines.

(Perhaps you had already come to something along these lines, but I do think providing some context in docs of the use case for this is valuable.) Cheers!

Why even worry about this? The simple and safe thing to do is kill the thread. Creating a new thread is cheap enough. I doubt that threads are being tainted at a high rate, so the cost of creating a new thread is insignificant in this context.

Sure. To be clear, I'm not suggesting that user's are encouraged to try to reset thread state. I agree that tracking / observing all potential those changes is highly complex and dependent on operating environment. The alternative of just leading thread be reaped / recreate is almost certainly more efficient from runtime and development perspective.

I'm not sure how to give useful guidelines on this, though I'm open to suggestions.

I suppose the documentation could be a note on LockOSThread:

A goroutine's thread context can be isolated with LockOSThread to allow modifications to that thread's system-level context, such as namespace, uid, gid, etc. Leaving the OS Thread locked at the completion of the goroutine indicates that the thread is polluted and should not be reused by other goroutines.

(Perhaps you had already come to something along these lines, but I do think providing some context in docs of the use case for this is valuable.) Cheers!

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Aug 2, 2017

Member

@benschumacher, I took a stab at your suggestion here: https://go-review.googlesource.com/c/52871

Member

aclements commented Aug 2, 2017

@benschumacher, I took a stab at your suggestion here: https://go-review.googlesource.com/c/52871

@markdascher

This comment has been minimized.

Show comment
Hide comment
@markdascher

markdascher Aug 26, 2017

May also be worth auditing references to UnlockOSThread to make sure they're being used appropriately. This one from ensureSigM seems debatable. (The goroutine never exits, as far as I can tell. But if it did, you wouldn't want the thread to be reused.)

May also be worth auditing references to UnlockOSThread to make sure they're being used appropriately. This one from ensureSigM seems debatable. (The goroutine never exits, as far as I can tell. But if it did, you wouldn't want the thread to be reused.)

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 6, 2017

Change https://golang.org/cl/68750 mentions this issue: runtime: rename sched.mcount to sched.mnext

Change https://golang.org/cl/68750 mentions this issue: runtime: rename sched.mcount to sched.mnext

gopherbot pushed a commit that referenced this issue Oct 11, 2017

runtime: replace sched.mcount int32 with sched.mnext int64
Currently, since Ms never exit, the number of Ms, the number of Ms
ever created, and the ID of the next M are all the same and must be
small. That's about to change, so rename sched.mcount to sched.mnext
to make it clear it's the number of Ms ever created (and the ID of the
next M), change its type to int64, and use mcount() for the number of
Ms. In the next commit, mcount() will become slightly less trivial.

For #20395.

Change-Id: I9af34d36bd72416b5656555d16e8085076f1b196
Reviewed-on: https://go-review.googlesource.com/68750
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

gopherbot pushed a commit that referenced this issue Oct 11, 2017

runtime: make it possible to exit Go-created threads
Currently, threads created by the runtime exist until the whole
program exits. For #14592 and #20395, we want to be able to exit and
clean up threads created by the runtime. This commit implements that
mechanism.

The main difficulty is how to clean up the g0 stack. In cgo mode and
on Solaris and Windows where the OS manages thread stacks, we simply
arrange to return from mstart and let the system clean up the thread.
If the runtime allocated the g0 stack, then we use a new exitThread
syscall wrapper that arranges to clear a flag in the M once the stack
can safely be reaped and call the thread termination syscall.

exitThread is based on the existing exit1 wrapper, which was always
meant to terminate the calling thread. However, exit1 has never been
used since it was introduced 9 years ago, so it was broken on several
platforms. exitThread also has the additional complication of having
to flag that the stack is unused, which requires some tricks on
platforms that use the stack for syscalls.

This still leaves the problem of how to reap the unused g0 stacks. For
this, we move the M from allm to a new freem list as part of the M
exiting. Later, allocm scans the freem list, finds Ms that are marked
as done with their stack, removes these from the list and frees their
g0 stacks. This also allows these Ms to be garbage collected.

This CL does not yet use any of this functionality. Follow-up CLs
will. Likewise, there are no new tests in this CL because we'll need
follow-up functionality to test it.

Change-Id: Ic851ee74227b6d39c6fc1219fc71b45d3004bc63
Reviewed-on: https://go-review.googlesource.com/46037
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

@gopherbot gopherbot closed this in 4f34a52 Oct 11, 2017

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Jan 2, 2018

Change https://golang.org/cl/85662 mentions this issue: runtime: remove special handling of g0 stack

Change https://golang.org/cl/85662 mentions this issue: runtime: remove special handling of g0 stack

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch Jan 26, 2018

@aclements: I'm reading the release notes for go1.10rc1 about locked OS threads. This part in particular has me concerned:

Because one common use of LockOSThread and UnlockOSThread is to allow Go code to reliably modify thread-local state (for example, Linux or Plan 9 name spaces), the runtime now treats locked threads as unsuitable for reuse or for creating new threads.

I have code which locks the thread, unshares the mount namespace, mounts a file-system (which is now visible only in that thread) and calls os/exec to run commands which need access to the mounted file-system. The code relies on the forked process being in the same mount namespace as the thread.

Will this code break with the new behaviour? It's unclear from the release notes.

rgooch commented Jan 26, 2018

@aclements: I'm reading the release notes for go1.10rc1 about locked OS threads. This part in particular has me concerned:

Because one common use of LockOSThread and UnlockOSThread is to allow Go code to reliably modify thread-local state (for example, Linux or Plan 9 name spaces), the runtime now treats locked threads as unsuitable for reuse or for creating new threads.

I have code which locks the thread, unshares the mount namespace, mounts a file-system (which is now visible only in that thread) and calls os/exec to run commands which need access to the mounted file-system. The code relies on the forked process being in the same mount namespace as the thread.

Will this code break with the new behaviour? It's unclear from the release notes.

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Jan 26, 2018

Member

Hi @rgooch. This change is specifically to make things like that better. Prior to this change, random goroutines could wind up running in your new mount namespace because the runtime spawned new internal threads off your locked thread. Now that will no longer happen. But this doesn't affect the behavior of os/exec.

Maybe I misunderstand your concern? If the release notes aren't clear, I'd like to understand how to improve them. Is your specific interpretation of the release notes that this would affect the fork performed by os/exec because that, in some sense, creates a new thread? What we really mean is that the runtime will never clone another thread off of a locked thread for the purposes of scheduling goroutines.

Member

aclements commented Jan 26, 2018

Hi @rgooch. This change is specifically to make things like that better. Prior to this change, random goroutines could wind up running in your new mount namespace because the runtime spawned new internal threads off your locked thread. Now that will no longer happen. But this doesn't affect the behavior of os/exec.

Maybe I misunderstand your concern? If the release notes aren't clear, I'd like to understand how to improve them. Is your specific interpretation of the release notes that this would affect the fork performed by os/exec because that, in some sense, creates a new thread? What we really mean is that the runtime will never clone another thread off of a locked thread for the purposes of scheduling goroutines.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch Jan 26, 2018

Hi, @aclements. Correct, the release notes are not clear to me. From your explanation above, it seems that when using os/exec, the thread state of the calling goroutine is inherited, regardless of whether the goroutine called runime.LockOSThread (that's the behaviour I rely on). It would help if this were made clear.

rgooch commented Jan 26, 2018

Hi, @aclements. Correct, the release notes are not clear to me. From your explanation above, it seems that when using os/exec, the thread state of the calling goroutine is inherited, regardless of whether the goroutine called runime.LockOSThread (that's the behaviour I rely on). It would help if this were made clear.

@aclements

This comment has been minimized.

Show comment
Hide comment
@aclements

aclements Jan 26, 2018

Member

Yes, with os/exec, the thread state of the caller is inherited by the new process. Though it's still important to call runtime.LockOSThread because otherwise you have no control over which thread's state will be inherited.

Member

aclements commented Jan 26, 2018

Yes, with os/exec, the thread state of the caller is inherited by the new process. Though it's still important to call runtime.LockOSThread because otherwise you have no control over which thread's state will be inherited.

@rgooch

This comment has been minimized.

Show comment
Hide comment
@rgooch

rgooch Jan 26, 2018

OK, great. If you can tweak the documentation and release notes to make that clear, that will be helpful, thanks. The behaviour you've described is the one I rely on, and it's good to know I can keep relying on :-)

rgooch commented Jan 26, 2018

OK, great. If you can tweak the documentation and release notes to make that clear, that will be helpful, thanks. The behaviour you've described is the one I rely on, and it's good to know I can keep relying on :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment