Add initial works for websocket support #20
Conversation
There was a problem hiding this comment.
I have some notes, nothing too big. We may need to amend the WS portion of the spec so it is more consistent with the HTTP spec, but thats outside this PR.
One note I have would be to try and make PRs smaller so they are easier to go through. Its hard to judge what a good size is and it can change depending on the work being done, but for example, this could have been split into at least two PRs: one for the changes to the HTTP client stuff (e.g. all the naming updates, etc), and one to add in the WebSocket stuff. Not a huge deal, but it does make going through PRs easier and faster
| // WebSocketOptions is the config options for websocket protocol. | ||
| type WebSocketOptions struct { | ||
| // HandshakeTimeout specifies the duration for the handshake to complete. | ||
| HandshakeTimeout time.Duration `default:"45s"` |
There was a problem hiding this comment.
I don't have a sense for what a normal handshake duration is, but 45s seems sorta high. Where did you determine this value from?
There was a problem hiding this comment.
Yes, I should have made a note for this. There is a DefaultDialer in the gorilla/websocket pkg that we're using that specifies the default handshake timeout to be 45s. I don't have a sense on what is a good value either so I just use what they have there.
synse/http.go
Outdated
| } | ||
|
|
||
| // Open opens the websocket connection between the client and Synse Server. | ||
| // NOTE - this method is not applicable for http client. |
There was a problem hiding this comment.
I'd update the comment here just so we're not referencing websockets in the http client, maybe something like
Open opens the connection between the client and Synse Server. This fulfils the
Client interface, but has no effect for the httpClient.
|
|
||
| // Close closes the websocket connection between the client and Synse Server. | ||
| // NOTE - this method is not applicable for http client. | ||
| func (c *httpClient) Close() error { |
synse/synse.go
Outdated
| @@ -6,7 +6,12 @@ import ( | |||
| "github.com/vapor-ware/synse-client-go/synse/scheme" | |||
| ) | |||
|
|
|||
| // Client API for Synse Server. | |||
| // Client API for Synse Server, for both Websocket and HTTP client. | |||
| // FIXME - for the most part, Websocket and HTTP client expose similiar api | |||
There was a problem hiding this comment.
this is a good question. it may be worth opening a new issue for to track discussion around it. my initial instinct is to say that it is probably still worth keeping around, but i'll need to think about it more before I can really weigh in on this
synse/websocket.go
Outdated
| // counter counts the number of request sent. It has the type uint64 which can | ||
| // later be used by an atomic function, which makes it more concurrency-safe. | ||
| // FIXME - is it necessary though? | ||
| var counter uint64 |
There was a problem hiding this comment.
It is necessary for us to have some kind of atomic incremental counter so we can set the IDs correctly on the messages. This approach should work.
I will say that this variable should be moved out of the global scope and into the websocketClient scope so that it is tied to a single client's session. if its kept global and we initialize multiple sessions, they will all increment the same counter
synse/websocket.go
Outdated
| } | ||
|
|
||
| // Transactions returns the sorted list of all cached transaction IDs. | ||
| // NOTE - this method is not applicable for websocket client. |
There was a problem hiding this comment.
according to spec, this isn't supported here but I'm wondering if we should for consistency
| } | ||
|
|
||
| // Transaction returns the state and status of a write transaction. | ||
| func (c *websocketClient) Transaction(id string) (*scheme.Transaction, error) { |
There was a problem hiding this comment.
another spec issue.. if we support getting transaction status, we should probably also support async writes, since those are where the transaction statuses are returned..
synse/websocket.go
Outdated
| // getCounter safely gets current value of counter. | ||
| // func getCounter() uint64 { | ||
| // return atomic.LoadUint64(&counter) | ||
| // } |
There was a problem hiding this comment.
if unused, can be removed
synse/websocket.go
Outdated
| // makeRequest issues a request event, reads its response event and parse the | ||
| // response back in JSON. | ||
| // TODO - how async work in this case? is writeJSON and readJSON block or | ||
| // should it? do i need to verify returned response? how do i do that? |
There was a problem hiding this comment.
I'll need to do some research here. I know that we will need to verify the response, against the request and it may come back out of order, so we may need to do some additional req/resp tracking at the client level.
synse/websocket.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // verityResponse checks if the request/reponse metadata are matched. |
There was a problem hiding this comment.
verityResponse -> verifyResponse
|
Thanks for the comments. As for some of the spec issues that you mentioned above, I've been working on a branch here to fix these as I go through the proposal. Will open a PR for that soon. |
|
I have fixed most of the issues mentioned above within this PR scope. Others will be added in later PR. |
This PR adds in some initial work for websocket support (fixes #4 and #8). Some housekeeping and refactoring works will come in later PR.