Skip to content

feat: websocket server poc#13

Merged
moreirathomas merged 22 commits intomainfrom
feat/websocket-server-poc
Aug 2, 2022
Merged

feat: websocket server poc#13
moreirathomas merged 22 commits intomainfrom
feat/websocket-server-poc

Conversation

@moreirathomas
Copy link
Member

@moreirathomas moreirathomas commented Jul 26, 2022

Description

This PR upgrade the basic http server into a websocket server, allowing continuous and bi-directional communication with desktop.

The procedure flow is:

  • the server opens a http connection with the client and upgrade the connection to the websocket protocol
  • the server listens forever the incoming messages on that websocket
  • upon receiving messages, the server interprets the message and invoke the related method
  • on "run", the server starts the run: launches a goroutine that invokes Runner.Run(), pipes the progress as messages sends via the websocket. Then, when Runner.Run() returns, the server sends the result (or the error) to the client.
  • on "cancel", it cancels the run

Implementation details

At high level:

server.Handler handles websocket stuff. It has a single exported method, Handler.ServerHTTP, making it a http.Handler usable in cmd/server/main.go. We configure the handler via server.NewHandler (note: configuration from the desktop app may be injected from here).
The handler is the only one dealing with websocket directly. Serving on the correct path, it upgrades the connection, read incoming messages and invokes service's methods accordingly.

server.service interfaces the package runner.

Changes

  • package server now offers a websocket server (over a plain http one)
  • package socketio interfaces reading from and writing to a websocket away from struct server.service

Notes

  • support json message (input)
  • support json message (output)
  • custom type for messages (exhaustive list)
  • accept config as client input when starting the run
  • unit tests (another issue)
  • cleaner error handling (another issue)

Next steps suggestion

  • better error handling than current { event: 'error', error: 'reason as string' }
  • generate access token on the user machine and provide it to the server on start up
  • server.Handler restrict concurrent connections to /run to 1

Closes #12

@moreirathomas moreirathomas changed the title Feat/websocket server poc feat: websocket server poc Jul 26, 2022
@moreirathomas moreirathomas force-pushed the feat/websocket-server-poc branch from 28c7927 to bb72d28 Compare July 27, 2022 22:29
@moreirathomas moreirathomas added this to the MVP milestone Jul 27, 2022
@moreirathomas moreirathomas force-pushed the feat/websocket-server-poc branch from f1ad0ec to a394332 Compare July 27, 2022 22:41
@moreirathomas moreirathomas force-pushed the feat/websocket-server-poc branch from a394332 to f23c073 Compare July 27, 2022 22:46
@moreirathomas moreirathomas added must have Blocking feature for the targeted release scope:server labels Jul 28, 2022
server/handle.go Outdated
Comment on lines +46 to +63
inc := incomingMessage{}
err := reader.ReadJSON(&inc)
if err != nil {
log.Println(err)
break // Connection is dead.
}

// TODO Update package configparse for this purpose.
p, err := json.Marshal(inc.Data)
if err != nil {
log.Println(err)
break // Connection is dead.
}
cfg, err := configparse.JSON(p)
if err != nil {
log.Println(err)
break // Connection is dead.
}
Copy link
Member Author

Choose a reason for hiding this comment

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

We receive a config as JSON, but nested inside a message object. To decode the message we use websocket.Conn.ReadJSON(), which limits us given its signature: we have to give it the struct we want to decode the message into.

We already faced issues with JSON configs when parsing a config file, thus the package configparse internally uses unmarshaledConfig to decode in an intermediate struct.

Given the restriction of websocket.Conn.ReadJSON(), I needed to make the intermediate struct public to exploit it.

Just to remarshal it to parse it with configparse existing api.

I suspect we need to update configparse api to handle the new use case.

Maybe out of scope of this feature.

- Handler manages the websocket connection and interprets the messages
- service (private) interfaces package runner
@moreirathomas moreirathomas marked this pull request as ready for review July 30, 2022 17:06
Copy link
Member

@GregoryAlbouy GregoryAlbouy left a comment

Choose a reason for hiding this comment

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

Great!
Smooth integration with almost 0 change to the existing, also nice interfacing for socketio 👌

I left some remarks and suggestions, some of them can be addressed later (the sooner we merge this PR, the better!)

const port = "8080"
const (
port = "8080"
token = "6db67fafc4f5bf965a5a" //nolint:gosec
Copy link
Member

Choose a reason for hiding this comment

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

I'd be in favor of introducing actual authentication logics when they're specced rather than faking it now 🤔

Suggested change
token = "6db67fafc4f5bf965a5a" //nolint:gosec

Copy link
Member Author

Choose a reason for hiding this comment

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

Thing is we need to have a mean to go around the cors.

So the token is needed in this pr.

Are you in favor of not even passing it as a argument from main (where it will be passed from anyway) and in-line it in server?

Copy link
Member

Choose a reason for hiding this comment

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

We can still return true in CheckOrigin for now and implement the token logics later (I don't see how it is different from using a token that obtainable from the repo)

Regarding its implementation I'd keep the current one, except we get token from an environment variable instead of a hardcoded public value.

GregoryAlbouy
GregoryAlbouy previously approved these changes Aug 2, 2022
@moreirathomas moreirathomas merged commit d8adeae into main Aug 2, 2022
@moreirathomas moreirathomas deleted the feat/websocket-server-poc branch August 2, 2022 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

must have Blocking feature for the targeted release scope:server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose a websocket server to trigger a run, stop it, stream progression and provide the output

2 participants