Repository: open-telemetry/opamp-go
Pull request #348 (2025-02-11 14:28:15)

fix: wait for HTTP server serve() termination
 Fixes https://github.com/open-telemetry/opamp-go/issues/347

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/348?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) Report
Attention: Patch coverage is `56.25000%` with `7 lines` in your changes missing coverage. Please review.
> Project coverage is 78.19%. Comparing base [(`2ecac8d`)](https://app.codecov.io/gh/open-telemetry/opamp-go/commit/2ecac8d46319124440aef491c9d002c71011fb41?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) to head [(`e1c362a`)](https://app.codecov.io/gh/open-telemetry/opamp-go/commit/e1c362ab01257d3959a338875c1a70c31f994001?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).

| [Files with missing lines](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/348?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Patch % | Lines |
|---|---|---|
| [server/serverimpl.go](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/348?src=pr&el=tree&filepath=server%2Fserverimpl.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2VydmVyL3NlcnZlcmltcGwuZ28=) | 56.25% | [6 Missing and 1 partial :warning: ](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #348      +/-   ##
==========================================
- Coverage   78.29%   78.19%   -0.10%     
==========================================
  Files          25       25              
  Lines        2386     2398      +12     
==========================================
+ Hits         1868     1875       +7     
- Misses        410      414       +4     
- Partials      108      109       +1     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/348?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).

tigrannajaryan: Please update doc-comment of OpAMPServer.Stop() to explain that it waits until the server socket is released.
rogercoll: looks like there is an unrelated flaky test on the examples: 
`2025/02/13 14:49:28.332879 [OPAMP] Could not load TLS config, working without TLS: open ../../certs/certs/ca.cert.pem: no such file or directory`
rogercoll: related data race issue https://github.com/open-telemetry/opamp-go/issues/256
tigrannajaryan: > related data race issue #256

I rerun the build a couple times and it fails. Perhaps this PR changed timings and the race is now encountered more consistently. Regardless of the reason I can't merge the PR if the build does not pass. We need to fix #256 first.
rogercoll: @tigrannajaryan The race condition test is now green after merging main. There are some changes on the files related to formatting which I think it would be better to tackle in a separate PR: https://github.com/open-telemetry/opamp-go/pull/356
tigrannajaryan: > @tigrannajaryan The race condition test is now green after merging main. There are some changes on the files related to formatting which I think it would be better to tackle in a separate PR: #356

Excellent! Let's get #356 merged and then we will merge this one. Thank you for your patience and for going the extra mile.

Patch:
diff --git a/server/server.go b/server/server.go
index 46acff9..abcf538 100644
--- a/server/server.go
+++ b/server/server.go
@@ -73,8 +73,9 @@ type OpAMPServer interface {
 	// accept connections.
 	Start(settings StartSettings) error
 
-	// Stop accepting new connections and close all current connections. This should
-	// block until all connections are closed.
+	// Stop accepting new connections and close all current connections.
+	// This operation should block until both the server socket and all
+	// connections have been closed.
 	Stop(ctx context.Context) error
 
 	// Addr returns the network address Server is listening on. Nil if not started.
diff --git a/server/serverimpl.go b/server/serverimpl.go
index b6f2acd..de9055d 100644
--- a/server/serverimpl.go
+++ b/server/serverimpl.go
@@ -40,7 +40,8 @@ type server struct {
 
 	// The listening HTTP Server after successful Start() call. Nil if Start()
 	// is not called or was not successful.
-	httpServer *http.Server
+	httpServer        *http.Server
+	httpServerServeWg *sync.WaitGroup
 
 	// The network address Server is listening on. Nil if not started.
 	addr net.Addr
@@ -109,6 +110,9 @@ func (s *server) Start(settings StartSettings) error {
 		ConnContext: contextWithConn,
 	}
 	s.httpServer = hs
+	httpServerServeWg := sync.WaitGroup{}
+	httpServerServeWg.Add(1)
+	s.httpServerServeWg = &httpServerServeWg
 
 	listenAddr := s.httpServer.Addr
 
@@ -119,7 +123,10 @@ func (s *server) Start(settings StartSettings) error {
 		}
 		err = s.startHttpServer(
 			listenAddr,
-			func(l net.Listener) error { return hs.ServeTLS(l, "", "") },
+			func(l net.Listener) error {
+				defer httpServerServeWg.Done()
+				return hs.ServeTLS(l, "", "")
+			},
 		)
 	} else {
 		if listenAddr == "" {
@@ -127,7 +134,10 @@ func (s *server) Start(settings StartSettings) error {
 		}
 		err = s.startHttpServer(
 			listenAddr,
-			func(l net.Listener) error { return hs.Serve(l) },
+			func(l net.Listener) error {
+				defer httpServerServeWg.Done()
+				return hs.Serve(l)
+			},
 		)
 	}
 	return err
@@ -160,7 +170,16 @@ func (s *server) Stop(ctx context.Context) error {
 		defer func() { s.httpServer = nil }()
 		// This stops accepting new connections. TODO: close existing
 		// connections and wait them to be terminated.
-		return s.httpServer.Shutdown(ctx)
+		err := s.httpServer.Shutdown(ctx)
+		if err != nil {
+			return err
+		}
+		select {
+		case <-ctx.Done():
+			return ctx.Err()
+		default:
+			s.httpServerServeWg.Wait()
+		}
 	}
 	return nil
 }


Test patch:
diff --git a/server/serverimpl_test.go b/server/serverimpl_test.go
index b2d7062..5fa95f9 100644
--- a/server/serverimpl_test.go
+++ b/server/serverimpl_test.go
@@ -57,6 +57,36 @@ func TestServerStartStop(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestServerStartStopWithCancel(t *testing.T) {
+	srv := startServer(t, &StartSettings{})
+
+	err := srv.Start(StartSettings{})
+	assert.ErrorIs(t, err, errAlreadyStarted)
+
+	canceledCtx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	err = srv.Stop(canceledCtx)
+	assert.ErrorIs(t, err, context.Canceled)
+}
+
+func TestServerStartStopIdempotency(t *testing.T) {
+	endpoint := testhelpers.GetAvailableLocalAddress()
+	for i := 0; i < 10; i++ {
+		t.Run(fmt.Sprintf("Attempt #%d: ", i), func(t *testing.T) {
+			srv := startServer(t, &StartSettings{
+				ListenEndpoint: endpoint,
+			})
+
+			err := srv.Start(StartSettings{})
+			assert.ErrorIs(t, err, errAlreadyStarted)
+
+			err = srv.Stop(context.Background())
+			assert.NoError(t, err)
+		})
+	}
+}
+
 func TestServerStartStopWithMiddleware(t *testing.T) {
 	var addedMiddleware atomic.Bool
 	assert.False(t, addedMiddleware.Load())