Websocket protocol binding (only client) #606
Conversation
Signed-off-by: Francesco Guardiani <[email protected]>
Cleanup some stuff Signed-off-by: Francesco Guardiani <[email protected]>
slinkydeveloper
changed the title
|
|
||
| // UnsafeReceive is like Receive, except it doesn't guard from multiple invocations | ||
| // from different goroutines. | ||
| func (c *ClientProtocol) UnsafeReceive(ctx context.Context) (binding.Message, error) { |
There was a problem hiding this comment.
might not export this, seems like a footgun
There was a problem hiding this comment.
It's not a footgun, on the contrary this is expected for a websocket user to be unable to invoke multiple times from different goroutines a method to read from the same connection, because she/he knows the wasm stream is not multiplexed. I wish to keep it pub for "power users" of websockets that doesn't want this (huge) lock in front
There was a problem hiding this comment.
If you feel this is really problematic, we can make it unexported and export it later
|
|
||
| func consumeStream(reader io.Reader) { | ||
| //TODO is there a less expensive way to consume the stream? | ||
| ioutil.ReadAll(reader) |
There was a problem hiding this comment.
There might be some fancy seek thing we can do later... so we try to find the edge of a single event at a time
There was a problem hiding this comment.
We don't need to find the edge of the websocket message, this is already done by the lib itself. But yeah this is open to optimizations
|
LGTM I want to see the server side now :D |
| @@ -9,6 +9,7 @@ import ( | |||
| // Receiver receives messages. | |||
| type Receiver interface { | |||
| // Receive blocks till a message is received or ctx expires. | |||
| // Receive can be invoked safely from different goroutines. | |||
There was a problem hiding this comment.
I think this is a very big implication here, meaning that all the receive function in all protocol need to be checked to see if this is true.
There was a problem hiding this comment.
They already are, otherwise they couldn't pass the integration tests 😄
There was a problem hiding this comment.
This missing comment was more a mistake than a "new feature"
|
|
||
| var SupportedSubprotocols = []string{JsonSubprotocol} | ||
|
|
||
| func resolveFormat(subprotocol string) (format.Format, websocket.MessageType, error) { |
There was a problem hiding this comment.
The format has already supported different format types, are you planing to support different types? I am wondering if we can leverage or improve the existing framework.
sdk-go/v2/binding/format/format.go
Line 66 in 7bec7ba
There was a problem hiding this comment.
In sdk-go we support only the EventFormat atm, so here i'm just mapping to what we support currently
There was a problem hiding this comment.
Ah I see your point now, well this "mapping" of subprotocols is very specific to websockets protocol binding https://github.com/cloudevents/spec/blob/master/websockets-protocol-binding.md#15-cloudevents-subprotocols, i guess this code should live only in the websockets protocol. Also because it uses a websocket type (websocket.MessageType)
Signed-off-by: Francesco Guardiani <[email protected]>
Part of #598
Signed-off-by: Francesco Guardiani [email protected]