Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to pause/resume websocket without dumping subscriptions #1335

Merged
merged 7 commits into from
Aug 6, 2020
17 changes: 17 additions & 0 deletions Sources/ApolloWebSocket/WebSocketTransport.swift
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,23 @@ public class WebSocketTransport {

reconnect.value = oldReconnectValue
}

/// Disconnects the websocket while setting the auto-reconnect value to false,
/// allowing purposeful disconnects that do not dump existing subscriptions.
/// NOTE: You will receive an error on the subscription (should be a `Starscream.WSError` with code 1000) when the socket disconnects.
/// ALSO NOTE: To reconnect after calling this, you will need to call `resumeWebSocketConnection`.
public func pauseWebSocketConnection() {
self.reconnect.value = false
self.websocket.disconnect()
}

/// Reconnects a paused web socket.
///
/// - Parameter autoReconnect: `true` if you want the websocket to automatically reconnect if the connection drops. Defaults to true.
public func resumeWebSocketConnection(autoReconnect: Bool = true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think just reconnect is fine? Aligning it with Starscream. But on other hand adding auto can be more explicit. :P

self.reconnect.value = autoReconnect
self.websocket.connect()
}
}

// MARK: - HTTPNetworkTransport conformance
Expand Down
77 changes: 77 additions & 0 deletions Tests/ApolloWebsocketTests/StarWarsSubscriptionTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import Apollo
import ApolloTestSupport
@testable import ApolloWebSocket
import StarWarsAPI
import Starscream

class StarWarsSubscriptionTests: XCTestCase {
let SERVER: String = "ws://localhost:8080/websocket"
designatednerd marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -12,6 +13,8 @@ class StarWarsSubscriptionTests: XCTestCase {
var webSocketTransport: WebSocketTransport!

var connectionStartedExpectation: XCTestExpectation?
var disconnectedExpectation: XCTestExpectation?
var reconnectedExpectation: XCTestExpectation?

override func setUp() {
super.setUp()
Expand Down Expand Up @@ -408,11 +411,85 @@ class StarWarsSubscriptionTests: XCTestCase {

waitForExpectations(timeout: 10, handler: nil)
}

func testPausingAndResumingWebSocketConnection() {
let subscription = ReviewAddedSubscription()
let reviewMutation = CreateAwesomeReviewMutation()

// Send the mutations via a separate transport so they can still be sent when the websocket is disconnected
let httpTransport = HTTPNetworkTransport(url: URL(string: "http://localhost:8080/graphql")!)
let httpClient = ApolloClient(networkTransport: httpTransport)

func sendReview() {
let reviewSentExpectation = self.expectation(description: "review sent")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let reviewSentExpectation = self.expectation(description: "review sent")
let reviewSentExpectation = expectation(description: "review sent")

httpClient.perform(mutation: reviewMutation) { mutationResult in
switch mutationResult {
case .success:
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm how about already here it can fulfill the expectation? Or I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expectation should be fulfilled and the wait should be ended no matter which case comes in, so that's why it's below the switch statement

Copy link
Contributor

Choose a reason for hiding this comment

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

I think XCTFail runs before I guess. Hmm ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that's by design - we want that failure to occur before the wait ends.

case .failure(let error):
XCTFail("Unexpected error sending review: \(error)")
}

reviewSentExpectation.fulfill()
}
self.wait(for: [reviewSentExpectation], timeout: 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.wait(for: [reviewSentExpectation], timeout: 10)
wait(for: [reviewSentExpectation], timeout: 10)

}

let subscriptionExpectation = self.expectation(description: "Received review")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let subscriptionExpectation = self.expectation(description: "Received review")
let subscriptionExpectation = expectation(description: "Received review")

// This should get hit twice - once before we pause the web socket and once after.
subscriptionExpectation.expectedFulfillmentCount = 2
let sub = self.client.subscribe(subscription: subscription) { subscriptionResult in
designatednerd marked this conversation as resolved.
Show resolved Hide resolved
switch subscriptionResult {
case .success(let graphQLResult):
XCTAssertEqual(graphQLResult.data?.reviewAdded?.episode, .jedi)
subscriptionExpectation.fulfill()
case .failure(let error):
if let wsError = error as? Starscream.WSError {
// This is an expected error on disconnection, ignore it.
XCTAssertEqual(wsError.code, 1000)
} else {
XCTFail("Unexpected error receiving subscription: \(error)")
subscriptionExpectation.fulfill()
}
}
}

self.waitForSubscriptionsToStart()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.waitForSubscriptionsToStart()
waitForSubscriptionsToStart()

sendReview()

self.disconnectedExpectation = self.expectation(description: "Web socket disconnected")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.disconnectedExpectation = self.expectation(description: "Web socket disconnected")
disconnectedExpectation = expectation(description: "Web socket disconnected")

webSocketTransport.pauseWebSocketConnection()
self.wait(for: [self.disconnectedExpectation!], timeout: 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.wait(for: [self.disconnectedExpectation!], timeout: 10)
wait(for: [disconnectedExpectation!], timeout: 10)

Do we need to specify exact expectation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes since that's the only one we want to wait for


// This should not go through since the socket is paused
sendReview()

self.reconnectedExpectation = self.expectation(description: "Web socket reconnected")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.reconnectedExpectation = self.expectation(description: "Web socket reconnected")
reconnectedExpectation = expectation(description: "Web socket reconnected")

webSocketTransport.resumeWebSocketConnection()
self.wait(for: [self.reconnectedExpectation!], timeout: 10)
self.waitForSubscriptionsToStart()
Comment on lines +469 to +470
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.wait(for: [self.reconnectedExpectation!], timeout: 10)
self.waitForSubscriptionsToStart()
wait(for: [reconnectedExpectation!], timeout: 10)
waitForSubscriptionsToStart()


// Now that we've reconnected, this should go through to the same subscription.
sendReview()

self.wait(for: [subscriptionExpectation], timeout: 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.wait(for: [subscriptionExpectation], timeout: 10)
wait(for: [subscriptionExpectation], timeout: 10)


// Cancel subscription so it doesn't keep receiving from other tests.
sub.cancel()
}
}

extension StarWarsSubscriptionTests: WebSocketTransportDelegate {

func webSocketTransportDidConnect(_ webSocketTransport: WebSocketTransport) {
self.connectionStartedExpectation?.fulfill()
}

func webSocketTransportDidReconnect(_ webSocketTransport: WebSocketTransport) {
self.reconnectedExpectation?.fulfill()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.reconnectedExpectation?.fulfill()
reconnectedExpectation?.fulfill()

I don't know if there is a style guide, but I usually omit self when it's not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is not currently.

Personally, I find self makes it much easier to disambiguate local vars from class vars when you're looking at things without syntax highlighting (ex, in Github). At some point I do want to make this more consistent throughout the codebase, but I'm gonna leave these for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK yep, being consistent is the key here, I agree!

}

func webSocketTransport(_ webSocketTransport: WebSocketTransport, didDisconnectWithError error: Error?) {
self.disconnectedExpectation?.fulfill()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.disconnectedExpectation?.fulfill()
disconnectedExpectation?.fulfill()

}
}