From 112288ecb2969242ec7dad22dedbfedb70cff8e2 Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Thu, 20 Feb 2020 17:08:50 -0500 Subject: [PATCH] Unlock OS thread after netns is restored The current ns package code is very careful about not leaving the calling thread with the overridden namespace set, for example when origns.Set() fails. This is achieved by starting a new green thread, locking its OS thread, and never unlocking it. Which makes golang runtime to scrap the OS thread backing the green thread after the go routine exits. While this works, it's probably not as optimal: stopping and starting a new OS thread is expensive and may be avoided if we unlock the thread after resetting network namespace to the original. On the other hand, if resetting fails, it's better to leave the thread locked and die. While it won't work in all cases, we can still make an attempt to reuse the OS thread when resetting the namespace succeeds. This can be achieved by unlocking the thread conditionally to the namespace reset success. Signed-off-by: Ihar Hrachyshka --- pkg/ns/ns_linux.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/ns/ns_linux.go b/pkg/ns/ns_linux.go index 31ad5f62..a34f9717 100644 --- a/pkg/ns/ns_linux.go +++ b/pkg/ns/ns_linux.go @@ -178,7 +178,16 @@ func (ns *netNS) Do(toRun func(NetNS) error) error { if err = ns.Set(); err != nil { return fmt.Errorf("error switching to ns %v: %v", ns.file.Name(), err) } - defer threadNS.Set() // switch back + defer func() { + err := threadNS.Set() // switch back + if err == nil { + // Unlock the current thread only when we successfully switched back + // to the original namespace; otherwise leave the thread locked which + // will force the runtime to scrap the current thread, that is maybe + // not as optimal but at least always safe to do. + runtime.UnlockOSThread() + } + }() return toRun(hostNS) } @@ -193,6 +202,10 @@ func (ns *netNS) Do(toRun func(NetNS) error) error { var wg sync.WaitGroup wg.Add(1) + // Start the callback in a new green thread so that if we later fail + // to switch the namespace back to the original one, we can safely + // leave the thread locked to die without a risk of the current thread + // left lingering with incorrect namespace. var innerError error go func() { defer wg.Done()