[SDK-222] [no connection serial ] + implement missing internal presence map + presencequeue

[sacOO7]

• Fixed RTP2, RTP17, RTP19: PresenceMap #255

[lmars]

I've given high level review based on Go code alone, I defer to others with more context about the functional changes.

[SimonWoolf]

The naming of isAttachWithoutMessageLoss seems misleading. That'll be set to true in any situation where you need to re-enter your members, which includes both situations where you do and don't have message loss

[sacOO7]

Let's say, if you attached channel when previous state was detached or attaching, do we get resume as false in such a case?

[sacOO7]

So, we should just call it shouldEnterInternalMembers wdyt?

[sacOO7]

I couldn't find any meaningful name that can denote the cause for this boolean flag : (

[SimonWoolf]

Let's say, if you attached channel when previous state was detached or attaching, do we get resume as false in such a case?

That, well, depends whether you resumed, because that's what the flag tells you. So you can go from attaching->attached with resumed=true or from attaching->attached with resumed=true with resumed=false.

But in protocol v2, whether you resumed is irrelevant. RTP17f: you should perform automatic re-entry whenever a channel moves into the ATTACHED state (from a different state). It doesn't matter whether you resumed, you do it either way.

[SimonWoolf]

why does closed need to be special-cased? can't it be handled by connStateError(state) like all the others?

[sacOO7]

Actually

ably-go/ably/state.go

Lines 55 to 60 in e66ad6a

	var connStateErrors = map[ConnectionState]ErrorInfo{
	ConnectionStateInitialized: *errNeverConnected,
	ConnectionStateDisconnected: *errDisconnected,
	ConnectionStateFailed: *errFailed,
	ConnectionStateSuspended: *errSuspended,
	}

map doesn't contain errClosed.
At the same time, we can't update the map to include errClosed since it can be a graceful close from the client side.
If we update the map with errClosed, other tests start failing where we don't expect error when client goes into closed state
E.g. call to client.close(), and conn state changes to closed with error.
This error is specific to our use-case, so I didn't include it in the map

[SimonWoolf]

At the same time, we can't update the map to include errClosed since it can be a graceful close from the client side.

I don't follow. If the connection is closed, then most operations you try to do to it (other than reconnect) should fail with a 'connection closed' error, similarly to if it's failed. if connclosed is missing from that then either every place you call is going to need to check if the conn is closed and handle that explicitly, or you have a bug, no?

E.g. call to client.close(), and conn state changes to closed with error.

No it doesn't because connection state changes don't have errors. They have reasons. The reason might be 'the connection was closed', that's not an error, it's telling you the reason that the connection is in that state

[sacOO7]

I think there is errorReason =>

ably-go/ably/realtime_conn.go

Lines 48 to 51 in e66ad6a

	// errorReason is an [ably.ErrorInfo] object describing the last error received if
	// a connection failure occurs (RTN14a).
	errorReason *ErrorInfo

and same is copied into reason

ably-go/ably/realtime_conn.go

Lines 1001 to 1006 in e66ad6a

	change := ConnectionStateChange{
	Current: c.state,
	Previous: previous,
	Reason: c.errorReason,
	RetryIn: retryIn,
	}

There are tests which don't treat connection closed to be connection failure as such. I mean ideally, they shouldn't.
Because client.close() initiates it and gracefully closes the connection accordingly.

[sacOO7]

Do we have a spec item that specifies to set a reason when client explicitly sends a close message and connection is closed by server

[lmars]

This looks ok from my perspective, but I defer to @SimonWoolf to review the changes he requested

[sacOO7]

@lmars merging this PR into integration-2.0, since one more review required for recovery spec related commits. I will re-raise @SimonWoolf comments there