Use a random subscription id as rpc/v0 does on rpc/tendermint#235
Conversation
|
LGTM, but breaks API to clients (e-pm) and eris-db.js; this is okay as 0.12.0 is a big transfer, but we need to coordinate. Hence Im holding off merging and will ping @dennismckinnon @NodeGuy |
|
Contrary to what I had thought, 46657 did have a subscribe and unsubscribe websocket implementation here: https://github.com/eris-ltd/eris-db/blob/master/Godeps/_workspace/src/github.com/tendermint/tendermint/rpc/server/handlers.go#L331 This implementation only allows a single subscription to the same event from the same host. On the face of it that might seem okay, but it puts the onus on the client to maintain a global websocket connection pool per event and reuse/share the same one for the same event. This isn't terribly nice for writing less stateful Javascript apps and furthermore there is no warning that by re-registering the event will clobber any previous connections (they will no longer receive the event). This is because AddListener in go-events overwrites any existing callback: |
|
Your arguments are very strong; I will want to wait for develop to be green; then rerun this build and then Im happy to push forward on this. |
|
I'm a bit worried about breaking compatibility, but using this approach the worst that happens is we break unsubscribe (they pass event name rather than subscription id, it's a no-op) -- something we have to rely on the clients doing anyway. Subscribe will work as before. And both methods have identical signatures so Unsubscribe will fail silently -- not the end of the world. |
|
Actually I guess the JSON versions would break because the key name has changed from |
0086982 to
f632852
Compare
|
Rebased |
| return true, nil | ||
| } | ||
|
|
||
| func testSubscribe(t *testing.T) { |
There was a problem hiding this comment.
Is this being run? as it is not exported?
There was a problem hiding this comment.
Note this is not a *_test.go file.
It's following the same pattern as other methods in here, it's the testing method that the real one in https://github.com/silasdavis/eris-db/blob/f632852bb6e032292b9cbadb427b03c807ddb018/rpc/tendermint/test/client_ws_test.go#L241 calls.
Aside from following that pattern, the main reason I have it here is because go-wire doesn't work from *_test.go files it would seem. Something to do with the global var interface registration.
There was a problem hiding this comment.
Oh actually since I factored out readResult. I could probably get away with having it in client_ws_test.go. Still I'm fairly happy having it here since all the tests in client_ws_test.go are proxy functions to ones here. Mind you they need to be to centralise the HTTP/JSON logic.
There was a problem hiding this comment.
Points taken onboard.
|
LGTM |
…ription-id Use a random subscription id as rpc/v0 does on rpc/tendermint
Fixes #177.
Fixes #172.
Fixes #225.