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

chore: updates the server startup #45

Merged
merged 5 commits into from
Dec 2, 2024
Merged

chore: updates the server startup #45

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f356d9
fix: http server blocks
lesterli Nov 30, 2024
b4d2ec2
shutdown handling
lesterli Dec 1, 2024
334a144
assign server and revert go.mod
lesterli Dec 1, 2024
b2a466c
updates for comments
lesterli Dec 1, 2024
fb51d13
add TODO
lesterli Dec 2, 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
47 changes: 29 additions & 18 deletions server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
package server

import (
"context"
"fmt"
"net"
"net/http"
"sync"
"sync/atomic"
"time"

Expand All @@ -22,7 +22,7 @@
// Server is the main daemon construct for the finality gadget server. It
// handles spinning up both the gRPC and HTTP servers, the database, and any
// other components that the the finality gadget server needs to run.
type Server struct {

Check failure on line 25 in server/server.go

View workflow job for this annotation

GitHub Actions / lint_test / lint 

fieldalignment: struct with 112 pointer bytes could be 96 (govet)
proto.UnimplementedFinalityGadgetServer
fg finalitygadget.IFinalityGadget
cfg *config.Config
Expand All @@ -32,6 +32,9 @@

interceptor signal.Interceptor
started int32

grpcServer *grpc.Server
httpServer *http.Server
}

// NewFinalityGadgetServer creates a new server with the given config.
Expand Down Expand Up @@ -75,33 +78,30 @@
// the interrupt handler.
<-s.interceptor.ShutdownChannel()

// shutdown servers
s.grpcServer.GracefulStop()
s.httpServer.Shutdown(context.Background())

Check failure on line 83 in server/server.go

View workflow job for this annotation

GitHub Actions / lint_test / lint 

Error return value of `s.httpServer.Shutdown` is not checked (errcheck)
bap2pecs marked this conversation as resolved.
Show resolved Hide resolved

return nil
}

func (s *Server) startGrpcServer() error {
lis, err := net.Listen("tcp", s.cfg.GRPCListener)
listener, err := net.Listen("tcp", s.cfg.GRPCListener)
if err != nil {
return fmt.Errorf("failed to listen on %s: %w", s.cfg.GRPCListener, err)
}
defer lis.Close()

grpcServer := grpc.NewServer()
defer grpcServer.Stop()

proto.RegisterFinalityGadgetServer(grpcServer, s)

var wg sync.WaitGroup
wg.Add(1)
ready := make(chan struct{})
Copy link
Contributor

Choose a reason for hiding this comment

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

why replace WaitGroup to use channel?

also why is the channel name ready?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WaitGroup only signals goroutine started, not if server is ready to accept connections, "ready" is used b/c it is clear for the channel's purpose

Copy link
Contributor

Choose a reason for hiding this comment

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

WaitGroup only signals goroutine started, not if server is ready to accept connections

but how does it work in the past? we have always been using wg

"ready" is used b/c it is clear for the channel's purpose

ask Cursor. it's not a good name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how does it work in the past?

right, WaitGroup works in server setups, but my understanding is the key difference is channels can propagate info while WaitGroup can only signal completion

it's not a good name

yes, it is too generic, maybe listenerReady is better name

go func() {
s.logger.Info("RPC server listening", zap.String("address", lis.Addr().String()))

// Close the ready chan to indicate we are listening.
defer lis.Close()

wg.Done()
_ = grpcServer.Serve(lis)
s.logger.Info("gRPC server listening", zap.String("address", s.cfg.GRPCListener))
close(ready)
_ = grpcServer.Serve(listener)
}()
wg.Wait()
<-ready
s.grpcServer = grpcServer
return nil
}

Expand All @@ -119,9 +119,20 @@
ReadHeaderTimeout: 30 * time.Second,
}

s.logger.Info("Starting standalone HTTP server", zap.String("address", s.cfg.HTTPListener))
if err := httpServer.ListenAndServe(); err != nil && err != http.ErrServerClosed {
return fmt.Errorf("HTTP server failed: %w", err)
listener, err := net.Listen("tcp", s.cfg.HTTPListener)
if err != nil {
return fmt.Errorf("failed to create HTTP listener: %w", err)
}
bap2pecs marked this conversation as resolved.
Show resolved Hide resolved

ready := make(chan struct{})
go func() {
s.logger.Info("Starting standalone HTTP server", zap.String("address", s.cfg.HTTPListener))
close(ready)
if err := httpServer.Serve(listener); err != nil && err != http.ErrServerClosed {
s.logger.Error("HTTP server failed", zap.Error(err))
}
}()
<-ready
s.httpServer = httpServer
return nil
}
Loading