Skip to content
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

fix(server/v2): avoid server stop get call before start for multi components #22811

Merged
merged 8 commits into from
Dec 10, 2024
Merged

fix(server/v2): avoid server stop get call before start for multi components #22811

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
74d3a61
fix(server/v2): avoid server stop get call before start for multi com…
mmsqe Dec 10, 2024
357ccfb
reuse
mmsqe Dec 10, 2024
c85b843
Merge branch 'main' into nil_server
mmsqe Dec 10, 2024
70751b6
avoid failed to start gRPC-Gateway server on 1317 port conflict
mmsqe Dec 10, 2024
59bfdd9
ref
mmsqe Dec 10, 2024
9db84ba
telemetry
mmsqe Dec 10, 2024
b69e5cd
Apply suggestions from code review
mmsqe Dec 10, 2024
dbe76eb
grpcgateway
mmsqe Dec 10, 2024
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
16 changes: 10 additions & 6 deletions server/v2/api/rest/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ func New[T transaction.Tx](
}
}
srv.config = serverCfg

srv.httpServer = &http.Server{
Addr: srv.config.Address,
Handler: srv.router,
}
return srv, nil
}

Expand All @@ -69,11 +72,6 @@ func (s *Server[T]) Start(ctx context.Context) error {
return nil
}

s.httpServer = &http.Server{
Addr: s.config.Address,
Handler: s.router,
}

s.logger.Info("starting HTTP server", "address", s.config.Address)
if err := s.httpServer.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
s.logger.Error("failed to start HTTP server", "error", err)
Expand All @@ -89,6 +87,12 @@ func (s *Server[T]) Stop(ctx context.Context) error {
}

s.logger.Info("stopping HTTP server")
defer func() {
Copy link
Member

Choose a reason for hiding this comment

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

If httpServer is set in New how come we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we call Start again later after Stop, the old httpServer will fail since Shutdown was called.

Copy link
Member

@julienrbrt julienrbrt Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see, but why would you do that? What is the use case?
I think we should just add in the server component docs, like in the std http server docs something like:

Once Stop has been called on a server, it may not be reused.

Copy link
Member

@julienrbrt julienrbrt Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Before we wanted to support server reloading, so then it would have made sense, but due to the config reloading limitation, we haven't gone that way. I think it makes server components simpler to write if you know once you call stop you don't need to make it re-startable. So setting the server in New instead of Start does makes sense, but imho, we don't need to change anything else in Stop.

s.httpServer = &http.Server{
Addr: s.config.Address,
Handler: s.router,
}
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Reconsider server reinitialization in Stop()

The deferred reinitialization of the httpServer after shutdown could be problematic:

  1. It's unclear why the server needs to be recreated after shutdown
  2. This could lead to resource leaks if the server is repeatedly stopped and started
  3. The reinitialization might mask other underlying issues

Consider removing the deferred reinitialization:

func (s *Server[T]) Stop(ctx context.Context) error {
    if !s.config.Enable {
        return nil
    }

    s.logger.Info("stopping HTTP server")
-    defer func() {
-        s.httpServer = &http.Server{
-            Addr:    s.config.Address,
-            Handler: s.router,
-        }
-    }()
    return s.httpServer.Shutdown(ctx)
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
defer func() {
s.httpServer = &http.Server{
Addr: s.config.Address,
Handler: s.router,
}
}()
return s.httpServer.Shutdown(ctx)

return s.httpServer.Shutdown(ctx)
}

Expand Down
1 change: 1 addition & 0 deletions systemtests/system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,7 @@ func (s *SystemUnderTest) AddFullnode(t *testing.T, beforeStart ...func(nodeNumb
if tomlFile == "app.toml" && IsV2() {
file := filepath.Join(WorkDir, s.nodePath(nodeNumber), "config", tomlFile)
EditToml(file, func(doc *tomledit.Document) {
SetValue(doc, fmt.Sprintf("%s:%d", node.IP, DefaultApiPort+nodeNumber), "grpc-gateway", "address")
SetValue(doc, fmt.Sprintf("%s:%d", node.IP, DefaultRestPort+nodeNumber), "rest", "address")
})
}
Expand Down
Loading