v1.2 event emitter for connection (RTN4) #144
Conversation
It makes the interface dirtier, but this is for tests, and otherwise the test errors point to the wrong line.
And associated API fixes.
Failing to close a sandbox app or realtime connection at the end of a test just leaves some resources lingering around, but it doesn't make the test actually fail.
This required a way to fake DISCONNECTs.
|
@QuintinWillison I'm not sure how I should be assigning as reviewers for this work. |
|
@tcard will DM you on that. |
We were asserting the state to be a connection state, so it crashes with channels.
ably/error.go
Outdated
| @@ -135,3 +136,55 @@ func checkValidHTTPResponse(resp *http.Response) error { | |||
| } | |||
| return err | |||
| } | |||
|
|
|||
| // ErrorInfoV12 is an error produced by the Ably library. | |||
| type ErrorInfoV12 struct { | |||
There was a problem hiding this comment.
Can this not just be called ErrorInfo?
There was a problem hiding this comment.
I tried to explain in the PR description, but let me expand. I'm marking names that are spec-compliant by appending V12. This way, we can fix the external API in one go, once the compliant public API is implemented, by:
- Renaming every exported name that doesn't have a
V12suffix to be unexported. - Remove all
V12suffixes.
The main reason to do this this way is to make review easier, as it results in mostly additions, with as few changes as possible. But it also may help in keeping a separation between interface and implementation, as we're focusing on getting a stable interface first, then fixing what's underneath.
There was a problem hiding this comment.
The main reason to do this this way is to make review easier, as it results in mostly additions, with as few changes as possible
I actually think the review would be easier if this was just ErrorInfo like I expect from reading the spec, but if the V12 suffixes make the job of shipping the change easier then 👍
ably/error.go
Outdated
| see = "See " + errorHref | ||
| } | ||
| return fmt.Sprintf("[ErrorInfo :%s code=%d statusCode=%d] %s", e.Message, e.Code, e.StatusCode, see) | ||
| } |
There was a problem hiding this comment.
This looks identical to proto.ErrorInfo.Error, is there a reason to not just use proto.ErrorInfo? If there is a reason to have this ErrorInfo here too, could it not just embed proto.ErrorInfo?
There was a problem hiding this comment.
I don't see a reason to put ErrorInfo in a separate, internal-ish package. Per the spec, it's a public type. Ultimately, I guess one of them will go away, but I'm focusing first on getting the spec-compliant API out and then see how we can refactor things to deduplicate.
There was a problem hiding this comment.
Hmm ok, well should we at least add a TODO: remove duplication with proto.ErrorInfo comment?
I must admit though that I don't like the idea of creating more work for ourselves down the line rather than just doing the work now when we know it needs to be done (e.g. rather than copying and pasting code around, just move or share it).
ably/event_emitter.go
Outdated
| } | ||
| } | ||
|
|
||
| // On registers an event listener. If event is nil, all events will trigger |
There was a problem hiding this comment.
We should use an explicit value rather than nil to mean "all events" (e.g. ably.ConnectionEventAll) to make the usage more readable
There was a problem hiding this comment.
I decided to use nil in order to avoid deviating too much from the spec.
There was a problem hiding this comment.
I don't have a strong opinion about what consideration is more important, though.
There was a problem hiding this comment.
For me, allowing nil to express a desire for 'no filtering' in this case feels natural, especially if that matches what's in the spec.
Having said that, and given my lack of any experience in Go programming, nil sounds complicated so I may well be carrying assumptions with me from other languages.
There was a problem hiding this comment.
Ah ok, so I definitely had my Go hat on here where I always try to avoid overloading the semantics of nil to mean something other than the zero value of a pointer.
Looking at the spec, I see it declares that On should support two method signatures:
class EventEmitter<Event, Data>:
on((Data...) ->) // RTE4
on(Event, (Data...) ->) // RTE4
Given this spec covers so many different languages, I think it's incorrect to assume every language supports method overloading, but anyway, for Go I think we should implement two different methods rather than trying to represent overloading with nil:
func (e *EventEmitter) On(func(data)) { }
func (e *EventEmitter) OnEvent(event, func(data)) { }If you disagree, then keep the nil 🙂
There was a problem hiding this comment.
That's a good point. Translating overloads from the spec as different methods rather than by adopting a "lowest common denominator" kind of thing with nils sounds good to me.
Done at f6a700e.
ably/realtime_client.go
Outdated
| // The RealtimeV12 libraries establish and maintain a persistent connection | ||
| // to Ably enabling extremely low latency broadcasting of messages and presence | ||
| // state. | ||
| type RealtimeV12 = RealtimeClient |
There was a problem hiding this comment.
Given this is being merged into the integration/1.2 branch, I wonder whether we should just actively break the API in this branch to be compatible with the 1.2 spec, and then one day we just switch, make integration/1.2 be master and then maintain a deprecated release-1.1 branch with security fixes.
This would make the code easier to maintain (no need for multiple implementations of the same things in the same codebase but with V12 appended to their names), and makes upgrading to 1.2 a one step process (i.e. just update to the 1.2 branch in go.mod and continue to use ably.NewRealtimeClient but with newer features, rather than using ably.NewRealtimeClientV12 for a period of time and then in the future needing to go back and change it back to ably.NewRealtimeClient).
WDYT?
/cc @QuintinWillison how does this work in other repositories?
There was a problem hiding this comment.
I've not seen anything similar elsewhere so, as far as I know, this is new territory for Ably... though others have much more experience than me in how we do things here.
I'm not sure I entirely understand the problem in hand, to be honest. And, on top of that, I'm looking at the names NewRealtimeClientV12 and NewRealtimeClient and wondering why they've even got a New prefix in the first place!? Those names already make it feel like more than a warm start, if that makes sense!
There was a problem hiding this comment.
I really don't think we should be annotating type or function names with the version. If we're being forced to make a breaking change because what we had was broken in a way that prevents us being compatible, then we have to have a step where we force an upgrade and require the user to make changes.
I'm looking at the names NewRealtimeClientV12 and NewRealtimeClient and wondering why they've even got a New prefix in the first place!?
That's how factory functions are conventionally named in go.
There was a problem hiding this comment.
I'm not sure I entirely understand the problem in hand
So the main issue is maintaining backwards compatibility with people who may need to upgrade the ably-go dependency for reasons other than needing the new API we are shipping.
For example, if we push a security fix, people should be able to upgrade easily to take advantage of that fix without needing to also refactor their code because we also broke the API.
The way people upgrade in Go is typically by just pulling in master with go get -u, although recently Go got native module support which makes it easier to pull in changes from a specific branch (e.g. go get -u github.com/ably/[email protected]).
In terms of us maintaining this backwards compatibility, we have two options:
-
have a single git branch that supports both the new and the old API (which is what @tcard is doing in this PR so far) and expect users to specify in their code which version they are using (e.g.
ably.NewRealtimeClientV12()) -
have a branch per potentially incompatible version (e.g.
release-1.1andrelease-1.2) and expect users to specify in their dependency file (i.e.go.mod) which version of the API they are using, and then use that in their code
Personally, I think 2) is easier to maintain, and makes a clear difference between upgrading for a security fix and expecting no breaking changes (i.e. pulling in the latest commits from the release-1.1 branch) vs upgrading for a new feature and expecting to change your code accordingly (i.e. switching to the release-1.2 branch, or master).
I also think we should have some form of policy we follow across repositories for this.
There was a problem hiding this comment.
I really don't think we should be annotating type or function names with the version.
Just to clarify, this is temporary and won't be there in any public-facing release. See this comment for rationale.
There was a problem hiding this comment.
About backwards compatibility, basically what @lmars says is ultimately the plan here, except following Go modules format for version tags. And, of course, master will point to the newest release.
|
The discussions on this PR are really valuable in terms of defining the architectural trajectory of Ably's Go client library going forward. Talking with @paddybyers earlier, we both think that it would be helpful to get as much input as possible from Go programmers - both internal and those who have been involved with using the library externally to Ably. So, with that in mind, I'm ping'ing a few people who I know could have an interest if they have the time to do so. Please feel free to ignore this call but any input would be really valuable to us. Thanks! |
As suggested by @lmars at #144 (comment)
|
@lmars @QuintinWillison @paddybyers PTAL and please let me know if there's anything blocking left. @QuintinWillison That's a good idea. But maybe it shouldn't block the PR from being merged? We can accept this as "good enough" and keep improving with new suggestions before the release. |
tcard
changed the title
|
Thanks for the ping, I think @lmars and @tcard are doing a great job discussing how to best implement the spec and aligning on a best path forward. As noted in my other comment I am happy that a callback approach was chosen. Go makes it very simple to orchestrate blocking apis, and as Dave Cheney said, leave concurrency to the caller. |
The new-API Close wasn't doing the same as the old-API Close: it wasn't closing the underlying connection after the Ably-level CLOSE exchange, and it wasn't handling failure. Now these effects are moved to the connection event loop, as 1. Close is asynchronous on its effects, per the spec, and 2. those effects must happen every time a CLOSED or ERROR arrives, not just when calling the public-facing Close method. WebSocket-level errors remain unhandled and should be dealt with in a centralized way. In order to keep tests that rely on the old behavior, a FullRealtimeCloser adapter has been added.
Also, proto.ErrorInfo is no longer a Go error; it just holds information sent by the server in order to construct real errors as public ErrorInfos. The logic for decorating error messages from ErrorInfos has moved to the public ErrorInfo, as that's the one clients will be seeing.
genericError was meant as a wrapper of builtin error, but wrapping interface types doesn't work as intended. Anyway, it really isn't necessary.
This goroutine was never killed, and there's really no proper place to kill it. It should live as long as the channel instance lives, but the channel instance is garbage-collected. We could use runtime.SetFinalizer to tie the goroutine's lifetime to the instance's lifetime, but it's simpler to just not have the goroutine in the first place. So we replace the channel-based broadcast with callback-based broadcast, so that the listener (now just a function) is garbage-collected too. Eventually, we can use the callback-based EventEmitter to cover this in a cleaner way. This should be part of #144.
|
@lmars I believe I've addressed all feedback. Could you please resolve the conversations if you think they're addressed? |
ably/realtime_client.go
Outdated
| return c.Connection.Close() | ||
| // ConnectV12 is the same as Connection.Connect. | ||
| func (c *Realtime) ConnectV12() { | ||
| c.Connection.ConnectV12() |
There was a problem hiding this comment.
Sorry, I got back to this after a long while and mistakenly thought I already finished with this. Should've checked. Done now at 7da5dbe.
ably/realtime_conn.go
Outdated
| return c.connect(true) | ||
| } | ||
|
|
||
| // ConnectV12 attempts to move the connection to the CONNECTED state, if it | ||
| // can and if it isn't already. | ||
| func (c *Connection) ConnectV12() { |
ably/realtime_conn.go
Outdated
| // OnV12 registers an event handler for connection events of a specific kind. | ||
| // | ||
| // See package-level documentation on Event Emitter for details. | ||
| func (c *Connection) OnV12(e ConnectionEventV12, handle func(ConnectionStateChangeV12)) (off func()) { |
There was a problem hiding this comment.
I've left a couple of comments so far about the V12 suffixes, are these just left over that need to be cleaned up or are we keeping some of them?
Added ablytest.ConnWaiter helper to get the previous Result-based behavior.
The old API stays for tests, for now. It will be easier to do once channels get their EventEmitter too and we can make ably.State private.
Setting off and calling it were concurrent, so use a channel instead.
And associated API fixes and test helpers.
As discussed, the spec-compliant API is being appended alongside the old API. We identify the spec-compliant API by appending a
V12to names. This way, we can work without having to refactor existing, working code, and when we're finished we can make the old API private and remove theV12prefixes in a single automated swoop.The RTN4a test for SUSPENDED is left failing until connection suspension is implemented.