-
Notifications
You must be signed in to change notification settings - Fork 13
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.
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
Quit event #1523
base: master
Are you sure you want to change the base?
Quit event #1523
Changes from 5 commits
802d324
f21487c
388e223
c8bd32e
e727aef
88781b1
35387f4
0dba117
7e65a9c
efe6f6c
00fcc0b
c7b321f
aa8fda3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,13 +3,15 @@ package server | |
import ( | ||
"context" | ||
"net" | ||
"net/http" | ||
"strconv" | ||
"time" | ||
|
||
"github.com/99designs/gqlgen/graphql/handler" | ||
"github.com/99designs/gqlgen/graphql/handler/extension" | ||
"github.com/99designs/gqlgen/graphql/handler/lru" | ||
"github.com/99designs/gqlgen/graphql/handler/transport" | ||
"github.com/gorilla/websocket" | ||
"github.com/labstack/echo/v4" | ||
"github.com/labstack/echo/v4/middleware" | ||
|
||
|
@@ -21,21 +23,23 @@ import ( | |
) | ||
|
||
type Server struct { | ||
shutdown context.CancelFunc | ||
cancel context.CancelFunc | ||
done chan bool | ||
graphServer *handler.Server | ||
listener net.Listener | ||
httpServer *echo.Echo | ||
port int | ||
} | ||
|
||
func New(cfg *config.Instance, shutdown context.CancelFunc) (*Server, error) { | ||
func New(cfg *config.Instance, cancel context.CancelFunc) (*Server, error) { | ||
listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
if err != nil { | ||
return nil, errs.Wrap(err, "Failed to listen") | ||
} | ||
|
||
s := &Server{shutdown: shutdown} | ||
s.graphServer = newGraphServer(cfg) | ||
s := &Server{cancel: cancel} | ||
s.done = make(chan bool, 1) | ||
s.graphServer = newGraphServer(cfg, s.done) | ||
s.listener = listener | ||
s.httpServer = newHTTPServer(listener) | ||
|
||
|
@@ -61,6 +65,12 @@ func (s *Server) Start() error { | |
return s.httpServer.Start(s.listener.Addr().String()) | ||
} | ||
|
||
func (s *Server) quit() { | ||
s.done <- true | ||
close(s.done) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't look quite right. As the done channel is non-blocking, it wouldn't wait for the reader to consume the true event, and just immediately close the channel. That of course, indicates the quite event, by waking up the channel, but most likely the client will not retrieve the I think that is okay though. And we should just remove line 69 (and the buffer size of 1 on the channel). |
||
s.cancel() | ||
} | ||
|
||
func (s *Server) Shutdown() error { | ||
logging.Debug("shutting down server") | ||
ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||
|
@@ -72,9 +82,17 @@ func (s *Server) Shutdown() error { | |
return nil | ||
} | ||
|
||
func newGraphServer(cfg *config.Instance) *handler.Server { | ||
graphServer := handler.NewDefaultServer(genserver.NewExecutableSchema(genserver.Config{Resolvers: resolver.New(cfg)})) | ||
graphServer.AddTransport(&transport.Websocket{}) | ||
func newGraphServer(cfg *config.Instance, done chan bool) *handler.Server { | ||
graphServer := handler.NewDefaultServer(genserver.NewExecutableSchema(genserver.Config{Resolvers: resolver.New(cfg, done)})) | ||
graphServer.AddTransport(&transport.Websocket{ | ||
Upgrader: websocket.Upgrader{ | ||
CheckOrigin: func(r *http.Request) bool { | ||
// For development. User proper CORS for prod | ||
return true | ||
}, | ||
}, | ||
KeepAlivePingInterval: 10 * time.Second, | ||
}) | ||
graphServer.SetQueryCache(lru.New(1000)) | ||
graphServer.Use(extension.Introspection{}) | ||
graphServer.Use(extension.AutomaticPersistedQuery{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you are returning the
r.done
channel which is the same for every subscriber to this quit event, at most one of the subscribers would receive thetrue
value. But probably none of them (see other comment). All of them would wake up, which we could argue is enough. In that case I would change the return channel to<-chan struct{}
though.If you actually want to return a
true
event on the channel, I think something like this would work here: