-
Notifications
You must be signed in to change notification settings - Fork 199
Game server improvements #403
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
Conversation
Using snap intead of warp/wai simply because this project is already building on top of snap. Also puts the server state handling into a different module than the main function. This now takes the usual server arguments. This will require changes to the https reverse proxy.
the idea is that some static HTML page loads this and presents it nicely. It might also be interesting to export these numbers in a prometheus-compatbile format.
and also start tallying total statistics (total connnections, total games)
There is really no point in lookup up the game id in the hash map on every event. Instead, have a hash map of `MVar Game`, and all the game-handling code only needs to have access to the `MVar Game`. Hashmap lookups now only happen once when creating a new game, once when joining, and once when the last player dropped. Also, `broadcast` is now more thread safe: It sends messages while the game is taking; this way, even if two threads try to broadcast something, it arrives in the same order. (One might want to use a separate `MVar ()` mutex inside the game to not block receiving messages, not sure about that). Another nice example of type-driven refactoring :-)
wsApp state pending = do | ||
conn <- WS.acceptRequest pending | ||
WS.forkPingThread conn 30 | ||
welcome conn state `catch` \e -> print (e :: SomeException) |
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.
Is this catch going to make errors go to stdout instead of the regular Snap error log?
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.
It’s going to stdout, I did not change anything about that. I am also not sure if the websocket-snap-integration allows logging to the snap log here.
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.
Asked for help at jaspervdj/websockets-snap#18
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.
I'm not sure we need anything as complex as that. Unless I'm getting something wrong, I think if you just remove this catch, then exceptions will be handled in the normal way, which will write them to Snap's error logs.
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.
The catch is there from before. If snap handles this properly, then we don’t need that.
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.
Just tried it. the server survives, but the exception is not logged. Which, I think, is fine.
return $ ServerState {..} | ||
|
||
|
||
-- | A snapp handler |
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.
snapp looks like a typo?
} deriving (Show, Generic) | ||
instance ToJSON CurrentStats | ||
|
||
data TotalStats = TotalStats |
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.
Other possibly interesting numbers:
- Total messages sent
- Total messages received
Feel free to defer for later, though.
|
||
-- Statistics | ||
|
||
data CurrentStats = CurrentStats |
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.
Another interesting one would be average and maximum number of connections per game. Feel free to defer for later, though.
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.
Just add all the stats you want to #390 and they will not be forgotten.
and at the end of the game, add that to a total count. (This is currently slightly easier than adding them as they go, as this way the game thread does not need acesses to the global total stats MVar. This can be changed, of course.) As a side effect, serialize the broadcast message only once, instead once per player. Duh. (Unless GHC did that for us, but I doubt it.) Part of google#390.
As the snap server handles that gracefully.
Merged and pushed. Thanks! |
JSON-interface for statistics, more efficient code, etc. (see individual commit messages.)