Unexpected WebSocketTransport connections

[RolandasRazma]

Bug report

I would have created PR bout it's potentially breaking change...

-[WebSocketTransport updateHeaderValues:]
-[WebSocketTransport updateConnectingPayload:]
"reconnects" not connected socket - it shouldn't - there is reason why it is not connected. At least it should check if it's connected and then reconnect, but even better it should do nothing and allow developer to decide what to do. Because if you updating both (headers and payload) you creating unnecessary reconnects, in some cases developer might not even want to connect on those changes (my case)

Steps to reproduce

https://github.com/apollographql/apollo-ios/blob/main/Sources/ApolloWebSocket/WebSocketTransport.swift#L303
https://github.com/apollographql/apollo-ios/blob/main/Sources/ApolloWebSocket/WebSocketTransport.swift#L308

[designatednerd]

I'm fine with a PR if you do the check if it's connected and then only reconnect if it's already connected version. I think that keeps the simplicity for people that are already connected but helps solve your issue.

[RolandasRazma]

that's only partial fix, if you do something like

webSocketTransport.updateHeaderValues(...)
webSocketTransport.updateConnectingPayload(...)

while connected, it will still do more connects/disconnects than expected.

maybe something like

webSocketTransport.updateHeaderValues(..., reconnectIfConnected: false)
webSocketTransport.updateConnectingPayload(..., reconnectIfConnected: true)

[RolandasRazma]

looking at reconnectWebSocket() potentially it has bug as well

webSocketTransport.pauseWebSocketConnection()
webSocketTransport.updateHeaderValues(...)
DispatchQueue.global().async {
    webSocketTransport.resumeWebSocketConnection()
}

what is value of reconnect ? It depends... on how fast websocket.disconnect() and websocket.connect() was

[designatednerd]

while connected, it will still do more connects/disconnects than expected.

Fair in that we don't have that disconnect/reconnect explicitly documented (which they should be), but making any of those changes won't take effect without disconnecting and reconnecting if you're already connected.

I'm fine with adding a reconnectIfConnected parameter to make this explicit.

[designatednerd]

Released as part of 0.43.0!