Feat: Automatic Stale connection cleanup by Andrew-Lees11 · Pull Request #48 · Kitura/Kitura-WebSocket

Andrew-Lees11 · 2018-07-18T15:11:27Z

This PR adds a connectionTimeout tune-able to Websocket service. This represents the time in seconds after which, if a connection is has sent no messages, the WebSocket server will send a ping to the connection. If a pong is not received in response, the connection will be closed. If connectionTimeout is nil, no connection cleanup will take place.

This is requested in issue #24 for cleaning up stale connections.

This is implemented through a lastFrameReceivedAt timestamp and a Timer which checks every connectionTimeout seconds to see if the connection has had any activity. If their has been no activity it sends a ping to the client and if by the next interval their is no response it closes the connection.

This has been tested by running a chat server, crashing a client connection and seeing it get cleaned up by the server.

ianpartridge · 2018-07-24T12:22:07Z

Sources/KituraWebSocket/WebSocketConnection.swift

+    private var resumed: Bool = false
+
+    deinit {
+        timer.setEventHandler {}


Won't accessing this lazy property mean that the DispatchSourceTimer is created during deinit, even if connectionTimeout is nil?

ianpartridge · 2018-07-24T14:13:21Z

Sources/KituraWebSocket/WebSocketConnection.swift


    private var active = true

+    private let timer: DispatchSourceTimer = DispatchSource.makeTimerSource()


Doesn't this mean every connection creates a DispatchSource, even if we don't use it? Can we only create one (in init()) if we need one?

ianpartridge · 2018-07-24T14:37:35Z

Sources/KituraWebSocket/WebSocketConnection.swift

+        self.service = service
+        if let connectionTimeout = service?.connectionTimeout {
+            timer = DispatchSource.makeTimerSource()
+            self.timerStart(connectionTimeout: connectionTimeout)


No need for self. on function calls.

Andrew-Lees11 · 2018-07-25T09:48:21Z

@bridger Would you be able to have a look as this PR to ensure it fixes your issue and see if you have any comments? Thanks

ianpartridge · 2018-07-25T12:54:45Z

Sources/KituraWebSocket/WebSocketConnection.swift

+        self.service = service
+        if let connectionTimeout = service?.connectionTimeout {
+            lastFrameReceivedAt = Date()
+            timer = DispatchSource.makeTimerSource()


It might be neater if timerStart() created the timer too? So this would be

self.timer = timerStart(connectionTimeout: connectionTimeout)

timerStart requires self and so needs all stored properties to be initialized before being called. This means we can't have it return the timer. We could have the timer as an optional var defaulted to nil but since it is not changed I thought this was better.

ianpartridge · 2018-07-25T14:06:47Z

Tests/KituraWebSocketTests/ConnectionCleanupTests.swift

+
+    static var allTests: [(String, (ConnectionCleanupTests) -> () throws -> Void)] {
+        return [
+            ("testSingleConnectionTimeOut", testSingleConnectionTimeOut),


Can you make the order of the elements in the array match the order of the functions in the file please.

ianpartridge · 2018-07-25T14:07:47Z

Tests/KituraWebSocketTests/ConnectionCleanupTests.swift

+            usleep(1500)
+            XCTAssertEqual(service.connections.count, 1, "Failed to create connection to service")
+            sleep(3)
+            XCTAssertEqual(service.connections.count, 1, "Stale connection was unexpectantly cleaned up")


Unexpectedly

ianpartridge · 2018-07-25T14:09:43Z

Tests/KituraWebSocketTests/ConnectionCleanupTests.swift

+            sleep(1)
+            self.sendFrame(final: true, withOpcode: self.opcodePing, withPayload: NSData(), on: socket)
+            sleep(1)
+            XCTAssertEqual(service.connections.count, 1, "Stale connection was unexpectantly cleaned up")


Unexpectedly

ianpartridge · 2018-07-25T14:11:53Z

Tests/KituraWebSocketTests/KituraTest.swift

        WebSocket.register(service: service, onPath: onPath ?? servicePath)
    }

+    func register(onPath: String? = nil, closeReason: WebSocketCloseReasonCode, testServerRequest: Bool = false, pingMessage: String? = nil, connectionTimeout: Int? = nil) -> TestWebSocketService {


Do we need this extra function? Could we make the register() function above return a service instead?

ianpartridge · 2018-07-25T14:12:25Z

Sources/KituraWebSocket/WebSocketService.swift

+    /// The time in seconds after which, if a connection is has sent no messages, the WebSocket server will send a ping to the connection. If a pong is not received in response, the connection will be closed. If connectionTimeout is nil, no connection cleanup will take place.
+    var connectionTimeout: Int? { get }
+}
+extension WebSocketService {


Newline above this please.

Does this need to be marked public or is it implicitly public?

It is implicitly public but i can declare it public if that makes the code clearer

ianpartridge · 2018-07-25T14:15:18Z

Sources/KituraWebSocket/WebSocketService.swift

+    var connectionTimeout: Int? { get }
+}
+extension WebSocketService {
+    var connectionTimeout: Int? {


Comment please.

Does this need to be marked public or is it implicitly public?

protocols cannot declare field access levels so that is set when something conforms to the protocol. The comment describing this is on the protocol connectionTimeout which is what gets displayed in Jazzy. I will add a comment describing the default implementation though.

ianpartridge · 2018-07-30T15:22:05Z

README.md


+#### Detect and close broken connections
+
+WebSocketService protocol has an optional Int property called `connectionTimeout`. This is the time in seconds that a connection must be unresponsive to be automatically closed by the server. If the WebSocket server has not received any messages in the first half of the timeout time it will ping the connection. If a pong is not received in the remaining half if the timeout, the connection will be closed with a 1006 (connection closed abnormally) status code. The `connectionTimeout` defaults to `nil`, meaning no connection cleanup will take place.  


if the timeout -> of the timeout

ianpartridge · 2018-07-30T15:22:17Z

Sources/KituraWebSocket/WebSocketService.swift

    ///                     the client sent the message to this `WebSocketService`
    func received(message: String, from: WebSocketConnection)
+
+    /// The time in seconds that a connection must be unresponsive to be automatically closed by the server. If the WebSocket server has not received any messages in the first half of the timeout time it will ping the connection. If a pong is not received in the remaining half if the timeout, the connection will be closed with a 1006 (connection closed abnormally) status code. The `connectionTimeout` defaults to `nil`, meaning no connection cleanup will take place.


if the timeout -> of the timeout

ianpartridge

LGTM!

Motivation: On Kitura-WebSocket Kitura/Kitura-WebSocket#48 introduces an automatic connection cleanup feature. Using this, `WebSocketService`s can configure an optional timeout value. If a connection remains idle for half of the configured timeout value, the server sends a heartbeat ping to the client. In the a subsquent time period again equal to half of the configured timeout value, if a pong is received on the connection, the latter is kept alive. If not, the connection is closed. In `Kitura-WebSocket-NIO`, we implement this using the `IdleStateHandler` which is configured with half of the configured timeout value. When we receive the first `IdleStateEvent`, we send a heartbeat ping to the client. If a pong is received before the next `IdleStateEvent`, the connection is kept alive. If the next `IdleStateEvent` is received before the pong, the connection is closed. This commit also includes a new client framework for testing. It is difficult to write the connection cleanup tests using the existing adhoc WebSocket client framework implemented in `KituraTest.swift` and `KituraTest+Frames.swift`. The new client framework is called `WebSocketClient`. It uses a callback model and is largely work-in-progress. We must move all of the existing tests to the new framework in due time.

Motivation: On Kitura-WebSocket Kitura/Kitura-WebSocket#48 introduces an automatic connection cleanup feature. Using this, `WebSocketService`s can configure an optional timeout value. If a connection remains idle for half of the configured timeout value, the server sends a heartbeat ping to the client. In a following time period equal to half of the configured timeout value, if a pong is received on the connection, the latter is kept alive. If not, the connection is closed. In `Kitura-WebSocket-NIO`, we implement this using the `IdleStateHandler` which is configured with half of the configured timeout value. When we receive the first `IdleStateEvent`, we send a heartbeat ping to the client. If a pong is received before the next `IdleStateEvent`, the connection is kept alive. If the next `IdleStateEvent` is received before the pong, the connection is closed. This commit also includes a new client framework for testing. It is difficult to write the connection cleanup tests using the existing adhoc WebSocket client framework implemented in `KituraTest.swift` and `KituraTest+Frames.swift`. The new client framework is called `WebSocketClient`. It uses a callback model and is largely work-in-progress. We must move all of the existing tests to the new framework in due time.

* Add automatic stale connection cleanup Motivation: On Kitura-WebSocket Kitura/Kitura-WebSocket#48 introduces an automatic connection cleanup feature. Using this, `WebSocketService`s can configure an optional timeout value. If a connection remains idle for half of the configured timeout value, the server sends a heartbeat ping to the client. In a following time period equal to half of the configured timeout value, if a pong is received on the connection, the latter is kept alive. If not, the connection is closed. In `Kitura-WebSocket-NIO`, we implement this using the `IdleStateHandler` which is configured with half of the configured timeout value. When we receive the first `IdleStateEvent`, we send a heartbeat ping to the client. If a pong is received before the next `IdleStateEvent`, the connection is kept alive. If the next `IdleStateEvent` is received before the pong, the connection is closed. This commit also includes a new client framework for testing. It is difficult to write the connection cleanup tests using the existing adhoc WebSocket client framework implemented in `KituraTest.swift` and `KituraTest+Frames.swift`. The new client framework is called `WebSocketClient`. It uses a callback model and is largely work-in-progress. We must move all of the existing tests to the new framework in due time. * Log error and close channel on unknown error * Propagate event if not IdleStateEvent

Andrew-Lees11 added 3 commits July 17, 2018 16:26

Added ConnectionTimeout for stale connections

802b907

add stale cleanup timer

d3a9679

call connectionClose finstead of closeConnection

a4a1d66

Andrew-Lees11 requested a review from ianpartridge July 18, 2018 15:11

Andrew-Lees11 mentioned this pull request Jul 18, 2018

Stale connections are never closed #24

Closed

added cleanup tests

b39c138

ianpartridge reviewed Jul 24, 2018

View reviewed changes

fixed missing socket close in tests

bb4ac82

ianpartridge reviewed Jul 24, 2018

View reviewed changes

made connection timer optional

6dc4c7a

ianpartridge reviewed Jul 24, 2018

View reviewed changes

Andrew-Lees11 added 4 commits July 24, 2018 15:42

removed unneeded self from init

d9a4e2e

added time delay before checking connections

f3885ee

only update lastFrameReceivedAt if timer set

7eaca8a

increased time before checking connection count

122e15b

add connectionTimeout to README

bec0d76

ianpartridge reviewed Jul 25, 2018

View reviewed changes

Andrew-Lees11 added 4 commits July 25, 2018 16:24

implemented Ians requested Changes

047eb63

added let to connectionTimeout in README

b7157c5

extended response check time

ef4eca3

half timeout time

9d2c07c

ianpartridge reviewed Jul 30, 2018

View reviewed changes

fix docs if -> of typo

b576a1c

ianpartridge approved these changes Jul 31, 2018

View reviewed changes

removed extra line

35a30c2

ianpartridge merged commit 231ee01 into master Jul 31, 2018

ianpartridge deleted the staleCleanup branch July 31, 2018 08:31

djones6 mentioned this pull request Sep 6, 2018

Fix: make extension to WebSocketService public #62

Merged

Andrew-Lees11 mentioned this pull request Sep 6, 2018

Add a lastFrameReceivedAt timestamp so that dead connections can be detected #43

Closed

bridger mentioned this pull request Jul 11, 2019

Not receiving all disconnected notifications Kitura/Kitura-WebSocket-NIO#38

Open

pushkarnk mentioned this pull request Jul 17, 2019

Add automatic stale connection cleanup Kitura/Kitura-WebSocket-NIO#40

Merged


		private var active = true

		private let timer: DispatchSourceTimer = DispatchSource.makeTimerSource()


		#### Detect and close broken connections

		WebSocketService protocol has an optional Int property called `connectionTimeout`. This is the time in seconds that a connection must be unresponsive to be automatically closed by the server. If the WebSocket server has not received any messages in the first half of the timeout time it will ping the connection. If a pong is not received in the remaining half if the timeout, the connection will be closed with a 1006 (connection closed abnormally) status code. The `connectionTimeout` defaults to `nil`, meaning no connection cleanup will take place.

Conversation

Andrew-Lees11 commented Jul 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew-Lees11 commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianpartridge left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants