fix ci daily

[alixander]

./ci/test.sh --race ./... fails due to race conditions in cli tests. They're almost certainly innocuous, but we should get that fixed up.

https://github.com/terrastruct/d2/actions/runs/10755823241/job/29828005682

[govindup63]

/attempt #2087

[alixander]

i don't have the app installed, but i'll make sure it goes to you if completed

[alixander]

@govindup63 any progress?

[govindup63]

I got assigned a personal project the very next day so...you may assign it to someone else if it is possible sorry for inconvenience

[alixander]

@CodeMaverick2 yes.

[azar-writes-code]

Hi @alixander , Is there any progress from @CodeMaverick2 , I can't see anything here, Can I get this assigned to work on this issue?

Thanks,

[alixander]

@azar-writes-code yours!

[zsbahtiar]

/attempt https://github.com/terrastruct/d2/issues/2087

[alixander]

all yours @zsbahtiar

[Mayank77maruti]

Hey @alixander is this issue open and the bounty placed on it is still on.

[alixander]

all yours @Mayank77maruti

[Mayank77maruti]

Logs and the Error

The error logs from the CI pipeline show a data race condition in the Serve function inside xhttp/serve.go.
https://github.com/terrastruct/util-go/blob/master/xhttp/serve.go

Log Breakdown:

race: ==================
race: WARNING: DATA RACE
race: Write at 0x00c0008f60f0 by goroutine 200:
race:   oss.terrastruct.com/util-go/xhttp.Serve()
race:       /home/runner/go/pkg/mod/oss.terrastruct.com/[email protected]/xhttp/serve.go:39 +0x3cc
race:   oss.terrastruct.com/d2/d2cli.(*watcher).goServe.func1()
race:       /home/runner/work/d2/d2/d2cli/watch.go:487 +0x7e
race:   oss.terrastruct.com/d2/d2cli.(*watcher).goFunc.func1()
race:       /home/runner/work/d2/d2/d2cli/watch.go:203 +0xfa
race: 
race: Previous read at 0x00c0008f60f0 by goroutine 201:
race:   oss.terrastruct.com/util-go/xhttp.Serve.func1()
race:       /home/runner/go/pkg/mod/oss.terrastruct.com/[email protected]/xhttp/serve.go:27 +0x2e
race:   net/http.(*Server).Serve()
race:       /opt/hostedtoolcache/go/1.23.4/x64/src/net/http/server.go:3320 +0x491
race:   oss.terrastruct.com/util-go/xhttp.Serve.func2()
race:       /home/runner/go/pkg/mod/oss.terrastruct.com/[email protected]/xhttp/serve.go:32 +0x4f
race: ==================

From this, we can see:

• Goroutine 200 (Writer) is modifying memory at 0x00c0008f60f0 inside xhttp.Serve()
• Goroutine 201 (Reader) is accessing the same memory at 0x00c0008f60f0
• These goroutines are running concurrently, leading to a race condition.
• The error occurs within the Serve function, which means the concurrent access issue is inside that function.

---

Cause of the Data Race

In the Serve function in xhttp/serve.go(https://github.com/terrastruct/util-go/blob/master/xhttp/serve.go), we have:

done := make(chan error, 1)

go func() {
    done <- s.Serve(l)
}()

• The issue is that s.Serve(l) is executing in a separate goroutine.
• The select statement below is waiting for either done to receive a value or for ctx.Done().
• When multiple goroutines attempt to modify ctx or call s.Shutdown(ctx), this leads to concurrent access to shared resources.

---

Proposed Solution: Introduce a Mutex for Synchronization

To prevent multiple goroutines from accessing shared resources (e.g., modifying ctx or calling s.Shutdown(ctx)), I would like to introduce a sync.Mutex.

Final Code: (https://github.com/terrastruct/util-go/blob/master/xhttp/serve.go)

// Package xhttp implements http helpers.
package xhttp

import (
	"context"
	"log"
	"net"
	"net/http"
	"sync"
	"time"

	"oss.terrastruct.com/util-go/xcontext"
)

func NewServer(log *log.Logger, h http.Handler) *http.Server {
	return &http.Server{
		MaxHeaderBytes: 1 << 18, // 262,144B
		ReadTimeout:    time.Minute,
		WriteTimeout:   time.Minute,
		IdleTimeout:    time.Hour,
		ErrorLog:       log,
		Handler:        http.MaxBytesHandler(h, 1<<20), // 1,048,576B
	}
}

func Serve(ctx context.Context, shutdownTimeout time.Duration, s *http.Server, l net.Listener) error {
	s.BaseContext = func(net.Listener) context.Context {
		return ctx
	}

	var mu sync.Mutex // Mutex to prevent concurrent access
	done := make(chan error, 1)

	go func() {
		mu.Lock()
		defer mu.Unlock()
		done <- s.Serve(l) // Ensuring Serve is called safely
	}()

	select {
	case err := <-done:
		mu.Lock()
		defer mu.Unlock()
		return err
	case <-ctx.Done():
		mu.Lock()
		defer mu.Unlock()
		ctx = xcontext.WithoutCancel(ctx)
		ctx, cancel := context.WithTimeout(ctx, shutdownTimeout)
		defer cancel()
		return s.Shutdown(ctx)
	}
}

@alixander PTAL Thanks

[alixander]

@Mayank77maruti please submit PR

[Mayank77maruti]

Hey @alixander , i need to make changes to https://github.com/terrastruct/util-go/blob/master/xhttp/serve.go so should I make pr in utils-go repo

[alixander]

yup!

[Mayank77maruti]

Hey @alixander Do review https://github.com/terrastruct/util-go/pull/25

[alixander]

Woot, CI daily is now green. https://github.com/terrastruct/d2/actions/runs/13314465355

@Mayank77maruti I've messaged Algora team about claiming this bounty since it didn't go through the Algora app: https://discord.com/channels/1040418099612098610/1040423036035268700/1339670850886176768

Shouldn't be long. If in 48 hours there's no progress, please email me at [email protected]

[algora-pbc]

@Mayank77maruti: You've been awarded a $50 bounty by terrastruct! 👉 Complete your Algora onboarding to collect the bounty.

[algora-pbc[bot]]

🎉🎈 @Mayank77maruti has been awarded $50! 🎈🎊

• Share on socials
• Give feedback