refactor pool #192

artemredkin · 2020-04-03T16:45:02Z

Ideas for connection pool refactoring. Main idea is that we need to simplify connection lifecycle. Right now its spread over many callbacks, that return some kind of potential action. I think this part can be simpler. There are basically two operations, polling a connection and releasing the connection. All magic should as much as possible be contained in those two methods.
One thing I'm still figuring out is how to simplify idle connection handling.
Closes #175 and #176

There are two things missing at the moment:

removing connections from pool and removing providers when they are idle (this is caught by the failing test)
there are some verifications needed for event loop preference (and possibly tests?)

Other things to check:

what about inflight connection requests that are not finished yet (+test?)
we assert on openedConnectionCount on deinit, so we good there, only question is should we cancel those requests on shutdown or just let them fail when we stop eventLoop
Code handling idle pool connections is tough to follow, but I haven't found a way to simplify it yet
Same for code handling when channel is inactive
we probably need to move IdlePoolConnectionHandler stuff to connection pool file, otherwise we add idle handler in one place and remove in another

artemredkin · 2020-04-03T16:49:44Z

cc @adtrevor

weissi

Thanks! I left a few comments but I'm not sure if removing the struct State is the right call here. It makes it quite possible that we're calling out to the user whilst we hold a lock which we must prevent from happening. In the new code, it's unclear to me what bits call out and what bits don't so I think struct State was actually a good idea.

Sources/AsyncHTTPClient/ConnectionPool.swift

weissi · 2020-04-03T16:54:15Z

Sources/AsyncHTTPClient/ConnectionPool.swift

-                assert(self.state.activity == .closing)
-                return self.state.availableConnections
+            self.lock.withLock {
+                self.availableConnections.forEach { $0.close() }


$0.close() might also be calling out

same as above

Sources/AsyncHTTPClient/ConnectionPool.swift

PopFlamingo · 2020-04-04T03:19:41Z

@artemredkin Thank you very much for this!

what about inflight connection requests that are not finished yet (+test?)
we assert on openedConnectionCount on deinit, so we good there, only question is should we cancel those requests on shutdown or just let them fail when we stop eventLoop

I think currently we can't rely on the event loop correctly notifying remaining futures that it is shutdown, IIRC this was the related issue: apple/swift-nio#1309, the best option would be to fail the requests on shutdown IMO, it also gives a more consistent behaviour between client owned EL and user provided EL I guess?
I think there is already a test for checking they are indeed canceled on client shutdown

Code handling idle pool connections is tough to follow, but I haven't found a way to simplify it yet

Same for code handling when channel is inactive

Tell me if something doesn't make sense, I'll hopefully still remember the reasoning behind them 🙂

we probably need to move IdlePoolConnectionHandler stuff to connection pool file, otherwise we add idle handler in one place and remove in another

Agreed, that's not clean enough

PopFlamingo · 2020-04-04T03:27:08Z

EDIT: It's actually an assertion in HTTPBin, so not too much problematic for now (and I think it existed prior to this)

I hit this assertion in testRaceNewRequestsVsShutdown:

Test Suite 'Selected tests' started at 2020-04-04 05:25:47.352
Test Suite 'AsyncHTTPClientTests.xctest' started at 2020-04-04 05:25:47.354
Test Suite 'HTTPClientTests' started at 2020-04-04 05:25:47.354
Test Case '-[AsyncHTTPClientTests.HTTPClientTests testRaceNewRequestsVsShutdown]' started.
Fatal error: ioOnClosedChannel: file /Users/adtrevor/Development/AsyncHTTPClient/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift, line 525
2020-04-04 05:25:48.502101+0200 xctest[1407:29744] Fatal error: ioOnClosedChannel: file /Users/adtrevor/Development/AsyncHTTPClient/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift, line 525

…ient into refactor_pool

Sources/AsyncHTTPClient/HTTPClient.swift

Sources/AsyncHTTPClient/ConnectionPool.swift

artemredkin · 2020-05-12T18:47:16Z

@swift-server-bot test this please

…utput

artemredkin · 2020-05-14T12:22:41Z

@swift-server-bot test this please

artemredkin · 2020-05-15T10:42:39Z

This also closes #188

weissi

@artemredkin awesome, that looks like a good improvement! Left some more comments.

Sources/AsyncHTTPClient/ConnectionPool.swift

weissi · 2020-05-15T17:42:20Z

Sources/AsyncHTTPClient/ConnectionPool.swift

+        indirect case parkAnd(Connection, Action)
+    }
+
+    struct ConnectionsState {


Should we maybe move the state machine to a separate file?

good idea, done, thanks!

Sources/AsyncHTTPClient/ConnectionPool.swift

weissi · 2020-05-15T17:47:46Z

Sources/AsyncHTTPClient/ConnectionPool.swift

    }

    var connectionProviderCount: Int {
-        return self.connectionProvidersLock.withLock {
-            self.connectionProviders.count
+        return self.lock.withLock {


do we need this connectionProviderCount property? Seems racy so I'd just remove it.

we use it one place - client deinit to assert that all providers are deleted (so no decisions based on it are made)

@artemredkin in deinit we don't need the lock. So we can just do

assert(self.providers.count == 0)

weissi · 2020-05-15T17:56:21Z

Tests/AsyncHTTPClientTests/ConnectionPoolTests.swift

+        var state = HTTP1ConnectionProvider.ConnectionsState(eventLoop: eventLoop)
+
+        var snapshot = state.testsOnly_getInternalState()
+        XCTAssertEqual(0, snapshot.availableConnections.count)


they look all great! Quick question: Why are we asserting on the (private) state and not the Actions that the state machine returns? Examples here: https://github.com/apple/swift-nio-examples/blob/master/backpressure-file-io-channel/Tests/BackpressureChannelToFileIOTests/StateMachineTest.swift#L267-L309

For the example above, I extended the Action to have lots of functions (one for each of the actions) like

@discardableResult func assertOpenFile(_ check: (String) throws -> Void, file: StaticString = #file, line: UInt = #line) -> Self { if case .openFile(let path) = self.main { XCTAssertNoThrow(try check(path)) } else { XCTFail("action \(self) not \(#function)", file: file, line: line) } return self }

And then you can just write

self.stateMachine.someInput() .assertActionIsX() .assertWhateverElse()

and we wouldn't need to expose the state. But again, I think that's something we can do in the future.

The benefit of asserting on the Action is that we can change the state machine representation to say an enum without rewriting all the tests.

we do assert on actions as well (not in this test specifically, but in other tests), I want to test not only action but the way state is changing. I test before the action to "show" what state I was expecting in this test and after to see if we arrived at it, also serves as a documentation of sorts for the test case

Hmm, but the tests shouldn't care how the state machine achieves its goal, assuming it does, right? When we refactor, we'll lose all the 'private state tests'.
The state machine is fully deterministic, so there isn't anything you can't test with just inputs & actions. The state is fully determined by the inputs that have been fed.

As long as most of the tests do only action assertions, I think there's no harm caused also asserting on private state but I think it's a bit of an anti-pattern.

I agree, but it can be a bit easier to test that when we aqcuire a connection that it is inside the waiters for example, than to get a series of transformations to aquire and then release some other connection and then test that our promise was succeeded

Github’s broken, i was meaning to thumbs UP only but fat fingered a DOWN too which I can’t remove now 🤪

weissi

Thank you! I think this is a good improvement and the remaining bits can be done iteratively from here on. Let’s get this in soon but maybe @Lukasa could give it a sanity check?

Lukasa

Some minor notes here.

Lukasa · 2020-05-18T07:19:58Z

Sources/AsyncHTTPClient/ConnectionPool.swift

-                }
-                return http1Provider
+                let provider = HTTP1ConnectionProvider(key: key, eventLoop: eventLoop, configuration: self.configuration, pool: self)
+                self.providers[key] = provider


Are we not missing a call to enqueue here?

Ok, after reading through the entire PR I finally found out why we aren't. Can we add a comment here that states that providers begin at +1?

Lukasa · 2020-05-18T07:22:05Z

Sources/AsyncHTTPClient/ConnectionPool.swift

-            self.state = State(eventLoop: eventLoop, parentPool: parentPool, key: key)
+    /// Sets idle timeout handler and channel inactivity listener.
+    func setIdleTimeout(timeout: TimeAmount?) {
+        _ = self.channel.pipeline.addHandler(IdleStateHandler(writeTimeout: timeout)).flatMap { _ in


Idle handlers should usually go first in the pipeline.

Even if the timeout handling handler is added immediately after it? I don't think there are any handlers that can (or should) handle timeouts in the pipeline at this point...

The reason they should go first in the pipeline is so that they observe the TCP writes/reads, rather than something more structured that may be delayed. Otherwise slow connections risk being killed for being idle, when in practice there's data flow occurring and just not being seen due to intermediate buffering.

This function is potentially callable at any time, so we'll risk fewer bugs by explicitly calling out that the idle state handler should go first. I'm fine for the actual timeout handler to go wherever.

Ah! That makes sense, done

Lukasa · 2020-05-18T07:22:48Z

Sources/AsyncHTTPClient/ConnectionPool.swift

-        var description: String {
-            return "HTTP1ConnectionProvider { key: \(self.key), state: \(self.state) }"
-        }
+extension Connection: Hashable {


Minor nit, but I'd rather not conform something to Hashable/Equatable if its definition of hash value and equality is identity.

Why? And what would you recommend instead?

Users explicitly testing for identity and explicitly using ObjectIdentifier as a hash key.

The main reason I object is that Swift pretty pervasively separates the idea of equality and identity. Indeed, the Equatable docs call this out directly. In particular, being equatable implies that two objects are substitutable. If two objects are only equal if they are literally the same object, we have a way to express that already and do not need to bring equatability into the mix.

I don't think there is a good way to use ObjectIdentifier is a Set, while I can add and test for contains and remove, since ObjectIdentifier is not generic, I cannot say leased.map { $0.close() }

Do you need a Set? Why not just Array?

@Lukasa and 4) (which I think may be best):

fileprivate struct LeasedConnectionKey: Hashable { private let connection: Connection func ==(...) {} func hash... }

Sure, works for me.

I do add and remove connections constantly, its added on every new http call and removed when call is finished. This is a result from a simple benchmark (adds, removes, searches) on a small (8 element) array/set:

name time std iterations ---------------------------------------- array 3208.0 ns ± 29.53 % 425528 set 1677.0 ns ± 52.07 % 827306

In this case set is almost twice as fast...

I think I'll go with 4

Yeah, the problem with Array will be the removals of elements that are not the last...

Lukasa · 2020-05-18T07:23:52Z

Sources/AsyncHTTPClient/ConnectionPool.swift

+                case .failure(let error):
+                    self.connectFailed(error: error, waiter: waiter)
+                }
+            }


This block is repeated a few times, it seems like we could factor it out a bit.

Good idea, done, thanks!

refactor pool

18a5bd6

artemredkin added the kind/enhancement Improvements to existing feature. label Apr 3, 2020

artemredkin added this to the 1.2.0 milestone Apr 3, 2020

artemredkin requested review from weissi and Lukasa April 3, 2020 16:45

revert test

8da635a

artemredkin mentioned this pull request Apr 3, 2020

flaky test: testRaceNewRequestsVsShutdown: Attempting to use HTTP1ConnectionProvider with unexpected state: closing (expected: [opened]) #176

Closed

weissi requested changes Apr 3, 2020

View reviewed changes

artemredkin commented Apr 3, 2020

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool.swift Outdated Show resolved Hide resolved

artemredkin added 2 commits April 3, 2020 20:41

review fixes - move callouts out of locks

110dc2a

restore some asserts

6c51cc6

artemredkin linked an issue Apr 3, 2020 that may be closed by this pull request

flaky test: testRaceNewRequestsVsShutdown: Attempting to use HTTP1ConnectionProvider with unexpected state: closing (expected: [opened]) #176

Closed

remove duplicate test

d5830a8

PopFlamingo reviewed Apr 4, 2020

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool.swift Show resolved Hide resolved

artemredkin mentioned this pull request Apr 4, 2020

update ci setup #193

Merged

artemredkin added 6 commits April 6, 2020 10:18

Merge branch 'master' into refactor_pool

4b48322

add empty provider removal

5bc7c36

Merge branch 'refactor_pool' of github.com:swift-server/async-http-cl…

de8a139

…ient into refactor_pool

fix warning and a race

8188902

Merge branch 'master' into refactor_pool

2be80a2

Merge branch 'refactor_pool' of github.com:swift-server/async-http-cl…

addd0cd

…ient into refactor_pool

artemredkin commented Apr 16, 2020

View reviewed changes

Sources/AsyncHTTPClient/HTTPClient.swift Show resolved Hide resolved

artemredkin marked this pull request as ready for review April 16, 2020 13:05

PopFlamingo mentioned this pull request Apr 18, 2020

Support NIO Transport Services - part 2 #184

Merged

artemredkin requested a review from weissi April 18, 2020 18:22

weissi reviewed Apr 18, 2020

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool.swift Outdated Show resolved Hide resolved

weissi reviewed Apr 18, 2020

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool.swift Outdated Show resolved Hide resolved

remove temp code

7655bf5

artemredkin added 6 commits May 13, 2020 10:28

add debug output for failing test

f4bfb33

fix formatting

68d1f97

add test debug to test it

5a46a2a

even more debug logging

2436d33

execute post-connect actions on channel event loop and remove debug o…

000e3ad

…utput

Merge branch 'master' into refactor_pool

e6f58c6

artemredkin added 2 commits May 15, 2020 11:34

make http compressor part of the static channel setup

13fbd6d

properly wait on channel setup complete

30c5936

artemredkin linked an issue May 15, 2020 that may be closed by this pull request

HTTPClient.Task.fail can release its associated connection more than once #188

Closed

artemredkin added 2 commits May 15, 2020 12:27

move setup success to after request sent

66d641a

fix test http server crash

4a9866b

weissi reviewed May 15, 2020

View reviewed changes

artemredkin added 2 commits May 15, 2020 20:12

review fixes

b8c51a8

Merge branch 'master' into refactor_pool

aa2d279

weissi approved these changes May 15, 2020

View reviewed changes

Lukasa requested changes May 18, 2020

View reviewed changes

review fixes

551c1f8

artemredkin mentioned this pull request May 18, 2020

New release for 1.1.2/1.2.0 #213

Closed

artemredkin added 2 commits May 18, 2020 16:46

review fix

e8fb614

fix compilation

554b71c

Lukasa approved these changes May 18, 2020

View reviewed changes

artemredkin merged commit 8adfdb9 into master May 18, 2020

artemredkin deleted the refactor_pool branch May 18, 2020 16:47

This was referenced May 18, 2020

HTTPClientInternalTests.testProxyStreaming fails with unclean shutdown #178

Closed

StreamWriter expects to be called on some internal EventLoop #200

Closed

refactor pool #192

refactor pool #192

Conversation

artemredkin commented Apr 3, 2020 • edited Loading

artemredkin commented Apr 3, 2020

weissi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PopFlamingo commented Apr 4, 2020 • edited Loading

PopFlamingo commented Apr 4, 2020 • edited Loading

artemredkin commented May 12, 2020

artemredkin commented May 14, 2020

artemredkin commented May 15, 2020

weissi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemredkin May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemredkin commented Apr 3, 2020 •

edited

Loading

PopFlamingo commented Apr 4, 2020 •

edited

Loading

PopFlamingo commented Apr 4, 2020 •

edited

Loading

artemredkin May 18, 2020 •

edited

Loading