Skip to content

[extension/httpforwarder] Shutdown should wait server exit #37735

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dmitryax merged 3 commits into from
Feb 20, 2025
Merged

[extension/httpforwarder] Shutdown should wait server exit #37735

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7638c56
[extension/httpforwarder] Shutdown should wait server exit
pjanotti Feb 6, 2025
521c025
Merge branch 'main' into fix-httpforwaredextension-shutdown
pjanotti Feb 13, 2025
962622c
Merge branch 'main' into fix-httpforwaredextension-shutdown
pjanotti Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion extension/httpforwarderextension/extension.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"io"
"net/http"
"net/url"
"sync"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/component/componentstatus"
Expand All @@ -23,6 +24,7 @@ type httpForwarder struct {
server *http.Server
settings component.TelemetrySettings
config *Config
shutdownWG sync.WaitGroup
Comment thread
dmitryax marked this conversation as resolved.
}

var _ extension.Extension = (*httpForwarder)(nil)
Expand All @@ -47,7 +49,9 @@ func (h *httpForwarder) Start(ctx context.Context, host component.Host) error {
return fmt.Errorf("failed to create HTTP Client: %w", err)
}

h.shutdownWG.Add(1)
go func() {
defer h.shutdownWG.Done()
Comment thread
dmitryax marked this conversation as resolved.
if errHTTP := h.server.Serve(listener); !errors.Is(errHTTP, http.ErrServerClosed) && errHTTP != nil {
componentstatus.ReportStatus(host, componentstatus.NewFatalErrorEvent(errHTTP))
}
Expand All @@ -60,7 +64,9 @@ func (h *httpForwarder) Shutdown(_ context.Context) error {
if h.server == nil {
return nil
}
return h.server.Close()
err := h.server.Close()
h.shutdownWG.Wait()
return err
Comment on lines +67 to +69

@bogdandrutu bogdandrutu Feb 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using Shutdown ?

@pjanotti pjanotti Feb 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of Shutdown vs Close is orthogonal to the issue being fixed in this PR: deterministic release of the port used by the component. In both cases there is still the need to wait the call to Serve(listener) to return.

I tend to prefer Close since, in certain cases Shutdown can take much longer given that it needs to wait existing connections to finish gracefully. That said the component owners will have more context if they want to change the current behavior in that regard.

@bogdandrutu bogdandrutu Feb 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may miss something:

// Shutdown gracefully shuts down the server without interrupting any
// active connections. Shutdown works by first closing all open
// listeners, then closing all idle connections, and then waiting
// indefinitely for connections to return to idle and then shut down.
// If the provided context expires before the shutdown is complete,
// Shutdown returns the context's error, otherwise it returns any
// error returned from closing the [Server]'s underlying Listener(s).

A.k.a because it returns the error from the listener that function that you wait for it must be done.

@pjanotti pjanotti Feb 6, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take into account the possible interleaves of the calls to Serve and Shutdown/Close. The ownership of listener is transferred to the http.Server when the call to Serve(listener) happens. However, since that happens in a goroutine it is possible that Shutdown is called before the http.Server took the ownership of listener. In this case the listener is not closed until the call to Serve happens. Serve adds a deferred call to listener.Close, even if Shutdown was already called, which ensures that the port is released by the time the method returns.

While this may be rather improbable in practice, it is better for the component to behave deterministically with the shutdown ensuring no left over goroutine and/or port.

@pjanotti pjanotti Feb 13, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bogdandrutu PTAL at reply above

}

func (h *httpForwarder) forwardRequest(writer http.ResponseWriter, request *http.Request) {
Expand Down