Skip to content
This repository was archived by the owner on May 13, 2022. It is now read-only.

Use a random subscription id as rpc/v0 does on rpc/tendermint#235

Merged
benjaminbollen merged 1 commit intohyperledger-archives:developfrom
silasdavis:use-subscription-id
Aug 23, 2016
Merged

Use a random subscription id as rpc/v0 does on rpc/tendermint#235
benjaminbollen merged 1 commit intohyperledger-archives:developfrom
silasdavis:use-subscription-id

Conversation

@silasdavis
Copy link
Contributor

Fixes #177.
Fixes #172.
Fixes #225.

@benjaminbollen
Copy link
Contributor

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

@silasdavis
Copy link
Contributor Author

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:

func (cell *eventCell) AddListener(listenerID string, cb eventCallback) {
    cell.mtx.Lock()
    cell.listeners[listenerID] = cb
    cell.mtx.Unlock()
}

@benjaminbollen
Copy link
Contributor

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.

@silasdavis
Copy link
Contributor Author

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.

@silasdavis
Copy link
Contributor Author

Actually I guess the JSON versions would break because the key name has changed from event to subscriptionId. The HTTP version will fail silently.

@silasdavis silasdavis force-pushed the use-subscription-id branch from 0086982 to f632852 Compare August 23, 2016 09:50
@silasdavis
Copy link
Contributor Author

Rebased

return true, nil
}

func testSubscribe(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this being run? as it is not exported?

Copy link
Contributor Author

@silasdavis silasdavis Aug 23, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Points taken onboard.

@benjaminbollen
Copy link
Contributor

LGTM

@benjaminbollen benjaminbollen merged commit 632b73c into hyperledger-archives:develop Aug 23, 2016
@silasdavis silasdavis deleted the use-subscription-id branch August 23, 2016 10:22
silasdavis pushed a commit to silasdavis/burrow that referenced this pull request Mar 9, 2019
…ription-id

Use a random subscription id as rpc/v0 does on rpc/tendermint
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants