From 376fcdf110dc9fbde309e8b2cf00c297a93f0a76 Mon Sep 17 00:00:00 2001
From: Laurent Demailly <ldemailly@gmail.com>
Date: Wed, 17 Jul 2024 09:36:16 -0700
Subject: [PATCH] review comments, thanks @ccoVeille for the thorough look

---
 console_logging.go      | 4 +++-
 goroutine/gid_test.go   | 8 ++++----
 goroutine/gid_tinygo.go | 2 +-
 logger.go               | 2 +-
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/console_logging.go b/console_logging.go
index 7dc3017..121401d 100644
--- a/console_logging.go
+++ b/console_logging.go
@@ -88,7 +88,9 @@ var (
 	Color = false
 )
 
-func IsValid(file *os.File) bool {
+// Is the file (e.g os.StdErr) Stat()able so we can detect if it's a tty or not.
+// If not we switch in init() to Stdout.
+func isValid(file *os.File) bool {
 	if file == nil {
 		return false
 	}
diff --git a/goroutine/gid_test.go b/goroutine/gid_test.go
index 96fc9dc..4693451 100644
--- a/goroutine/gid_test.go
+++ b/goroutine/gid_test.go
@@ -10,6 +10,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"testing"
 )
 
@@ -21,7 +22,7 @@ func TestID(t *testing.T) {
 	}
 	n := 1000000 // for regular go
 	if IsTinyGo {
-		n = 1000 // for tinygo
+		n = 1000 // for tinygo, it OOMs with 1000000 and we're only self testing that we get different increasing ids.
 	}
 	var wg sync.WaitGroup
 	for i := 0; i < n; i++ {
@@ -45,8 +46,7 @@ var testID int64
 func goid() int64 {
 	if IsTinyGo {
 		// pretty horrible test that aligns with the implementation, but at least it tests we get 1,2,3... different numbers.
-		testID++
-		return testID
+		return atomic.AddInt64(&testID, 1)
 	}
 	var buf [64]byte
 	n := runtime.Stack(buf[:], false)
@@ -58,7 +58,7 @@ func goid() int64 {
 	return id
 }
 
-var gotid int64
+var gotid int64 // outside of the function to help avoiding compiler optimizations
 
 func BenchmarkGID(b *testing.B) {
 	for n := 0; n < b.N; n++ {
diff --git a/goroutine/gid_tinygo.go b/goroutine/gid_tinygo.go
index 93e1605..f4ee02e 100644
--- a/goroutine/gid_tinygo.go
+++ b/goroutine/gid_tinygo.go
@@ -20,7 +20,7 @@ var (
 
 func ID() int64 {
 	task := uintptr(currentTask())
-	lock.Lock()
+	lock.Lock() // explicit minimal critical section without using defer, on purpose.
 	if id, ok := mapping[task]; ok {
 		lock.Unlock()
 		return id
diff --git a/logger.go b/logger.go
index 01851c3..8915c5d 100644
--- a/logger.go
+++ b/logger.go
@@ -174,7 +174,7 @@ func (l *JSONEntry) Time() time.Time {
 
 //nolint:gochecknoinits // needed
 func init() {
-	if !IsValid(os.Stderr) { // wasm in browser case for instance
+	if !isValid(os.Stderr) { // wasm in browser case for instance
 		SetOutput(os.Stdout) // this could also be invalid too but... we tried.
 	}
 	setLevel(Info) // starting value