Fixes issue #234 : ConnectionsState exposes a setter into internal state #311
Conversation
|
Can one of the admins verify this patch? |
4 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
@swift-server-bot test this please |
| return .lease(connection, waiter) | ||
| } | ||
|
|
||
| // If there is an opened connection on the same loop, lease it and park returned | ||
| if let found = self.availableConnections.firstIndex(where: { $0.channel.eventLoop === eventLoop }) { |
There was a problem hiding this comment.
the following two ifs were removed since they cannot be reached, we added and assert on line 156 instead
|
|
||
| // If specific EL is required, we have only two options - find open one or create a new one | ||
| if required, let found = self.availableConnections.firstIndex(where: { $0.channel.eventLoop === eventLoop }) { |
There was a problem hiding this comment.
the following block was remove since it cannot be reached, we replaced it with an assert on line 266
ya-pulser
force-pushed
the
issue_234_02_generics
branch
from
49c574f to
9d4a921
Compare
|
@swift-server-bot test this please |
ya-pulser
force-pushed
the
issue_234_02_generics
branch
from
9d4a921 to
4bb0fbb
Compare
| // Waiter can only exists if there was no capacity at aquire. If some connection | ||
| // is released when we have waiter it can only indicate that we should lease (if EL are the same), | ||
| // or replace (if they are different). But we cannot increase connection count here. | ||
| assert(!self.hasCapacity) |
There was a problem hiding this comment.
Does this code behave gracefully if this invariant is not true?
There was a problem hiding this comment.
Good question. Worst case scenario - we will replace the connection with a new one without checking for potentially available connections first. Though if this happens, it means that there is a bug in our code... What do you think we should do if for some reason we fail this?
| @@ -287,20 +261,12 @@ extension HTTP1ConnectionProvider { | |||
|
|
|||
| mutating func processNextWaiter() -> Action<ConnectionType> { | |||
| if let waiter = self.waiters.popFirst() { | |||
| let (eventLoop, required) = self.resolvePreference(waiter.preference) | |||
| // There should be no case where we have waiters and available connections. As soon as connection | |||
| // is released and can be available, it should immideately go to the next waiter. | |||
There was a problem hiding this comment.
Is this true in the face of event loop preferences? Is it not possible to have a waiter that is waiting for a connection on the appropriate event loop?
There was a problem hiding this comment.
Yes. We can only have waiters if there are no available connections, otherwise any new waiter will be leased a connection or a new connection will be created (in your specific example it will be replaced to be precise). The only way for connection to become available is to be released through the release method. If connection is released and it's active (meaning it could become "available"), it will be handled by a first branch in release. If the connection is not active, we will get here. But at this point the available invariant holds, released connection cannot become available, so the only course of action is to create a new one. Does that make sense?
There was a problem hiding this comment.
There are three other call sites for processNextWaiter:
connectFailed. This is somewhat similar - that failed connection will not be returned to the available set, since its already closed, so again, the only course of action is to create a new connection.remoteClose- same thingtimeout- same thing as well
There was a problem hiding this comment.
We need to be extremely clear about our reasoning in this code. The comment reads: "As soon as connection is released and can be available, it should immideately go to the next waiter." I have natural questions about this: if the waiter has expressed a loop preference, it may be possible that a released connection is unsuitable for that waiter, despite being valid and active. What happens in this case?
It's totally possible that the code is right, but the comment is at best incomplete.
There was a problem hiding this comment.
if the released connection is not suitable for the next waiter, it will be .replaced. How about the following comment instead:
This method is called in 4 cases:
1. from `release` when connection is inactive and cannot be re-used
2. from `connectFailed` when we failed to establish a new connection
3. from `remoteClose` when connection was closed by the remote side and cannot be re-used
4. from `timeout` when connection was closed due to idle timeout and cannot be re-used.
In all cases connection will not be `available`. Given that the waiter can only be present if there are no available connections (otherwise it would be leased a connection), where can be no situation where we can lease some available connection and the only course of action is to create a new one.
There was a problem hiding this comment.
where can be no situation where we can
There was a problem hiding this comment.
This method is called in following cases:
1. from `release` when connection is inactive and cannot be re-used
2. from `connectFailed` when we failed to establish a new connection
3. from `remoteClose` when connection was closed by the remote side and cannot be re-used
4. from `timeout` when connection was closed due to idle timeout and cannot be re-used.
In all cases connection, which triggered the transition, will not be in `available` state.
Given that the waiter can only be present in the pool if there were no available connections
(otherwise it would be leased a connection immediately), we do not see a situation when we can lease another
available connection, therefore the only course of action is to create a new connection for the waiter.
Complex comment highlights that logic is convoluted here, but I am not ready to propose an immediate improvement for the logic.
There was a problem hiding this comment.
Happy for the complex comment to go in.
…e so modification allowed only using exposed API. Motivation: Having a setter for internal state of ConnectionsState led to a subset of test testing invalid invariants, for example when we have at the same time an available connecion and a waiter, which is an invalid state of the system. Modifications: * test of ConnectionsState * ConnectionsState * ConnectionPool are modified so the state under tests is achieved only by a sequence of modifications invoked by state API. During modification some tests are eliminated as they were testing artificial state, which can not be achieved by exposed APIs. ConnectionsState is pruned from "replace" as in no valid state we can have a situation when we can "replace" a connection. Invalid invariants and tests are removed. Result: We do not have a way to modify state of the ConnectionsState by direct interaction with private state of the object. Getter on the state is considered harmless and used for tests only.
ya-pulser
force-pushed
the
issue_234_02_generics
branch
from
4bb0fbb to
f886980
Compare
|
elaborated comment for #311 (comment) is pushed into the code |
|
I did amend / force push which I am not sure is the best way if we have "squash" when we merge in this repository. Sorry. |
This PR is built on top of preparation work submitted as PR #310 which contains code clean up, before changes in this PR will affect tests and implementation of ConnectionsState.
Work with the tests has shown that some state invariants could be achieved only by direct manipulation with the state, so this proposal removes tests and code related to such "invalid" states.