mklaca commented on pull request #76:
URL: https://github.com/apache/qpid-broker-j/pull/76#issuecomment-865146813
Hi @alex-rufous ,
your suggested implementation of the requestConnectionSlot suffers from
multiple issues, it is difficult to fix them all.
The simple iteration over VirtualHost and Broker ConnectionLimitProviders
does not do the same job as VirtualHostConnectionLimiter.
```
@Override
public ConnectionSlot requestConnectionSlot(AMQPConnection<?> connection)
{
final Collection<VirtualHostConnectionLimitProvider> hostLimiters =
_virtualHost.getChildren(VirtualHostConnectionLimitProvider.class);
final Collection<BrokerConnectionLimitProvider> brokerLimiters =
_broker.getChildren(BrokerConnectionLimitProvider.class);
final List<? extends ConnectionLimitProvider> allLimiters = new
ArrayList<>()
allLimiters.addAll(hostLimiters);
allLimiters.addAll(brokerLimiters);
boolean success = false;
try
{
for (ConnectionLimitProvider limiter : allLimiters)
{
limiter.registerAndCheckLimit(connection);
}
success = true;
}
finally
{
if (success)
{
connection.addDeletedTask(()-> {
final Collection<VirtualHostConnectionLimitProvider>
hostLimiters =
_virtualHost.getChildren(VirtualHostConnectionLimitProvider.class);
final Collection<BrokerConnectionLimitProvider>
brokerLimiters = _broker.getChildren(BrokerConnectionLimitProvider.class);
final List<? extends ConnectionLimitProvider> allLimiters =
new ArrayList<>()
allLimiters.addAll(hostLimiters);
allLimiters.addAll(brokerLimiters);
for (ConnectionLimitProvider limiter : limiters)
{
limiter.deregister(connection);
}
})
}
else
{
for (ConnectionLimitProvider limiter : allLimiters)
{
limiter.deregister(connection);
}
}
}
}
```
I have to notice that 'decrement' and 'revert' are not same operations in
case of frequency limit. You need a special 'revert' method to handle
exceptions properly.
### Issues:
- **The issues connected with a configuration change:**
What does happen in following scenario?
1. 'requestConnectionSlot' collects all limiters.
2. The connection is registered in all collected limiters, it executes
register method on every limiter.
3. The user changes the configuration of the broker.
4. The delete task collects all limiters.
5. The connection is de-registered in all collected limiters.
**Examples:**
- There are two limiters (LimiterA, LimiterB) and then the user changes the
configuration, he adds a new LimiterC. The connection was register only in
LimiterA and LimiterB and so it has to be deregistered only in LimiterA and
LimiterB. Your proposed solution will also de-register the connection in the
LimiterC and that is a bug.
- There is just single limiter but the user decides to reset the counters,
the old counter inside the limiter is dropped and a new one is created.
'requestConnectionSlot' increments the old counter but the new incorrect
limiter is decremented in the delete task.
The connection could remember the limiters where it was registered but that
is memory consuming. If the user deleted a limiter then the limiter would
remain stored inside the connection.
- **The consistency issue:**
Suppose three limiters LimiterA, LimiterB, LimiterC and the following
scenario:
1. The method 'requestConnectionSlot' collects all limiters.
2. The connection is registered in LimiterA.
3. The registration of the connection in LimiterB fails and an exception is
thrown.
4. The connection is de-registered in LimiterA, LimiterB, LimiterC. That is
a bug, connection was registered only in LimiterA and so the change has to be
reverted only in LimiterA.
You need a special 'revert' method that properly reverts the changes and
checks that the connection was registered in the limiter indeed.
- **The concurrency issues:**
Every connection should be register in all limiters or it should not be
registered in any limiter. But the suggested implementation allows a lot of
intermediate states when the connection is 'half' registered or 'half'
de-registered. Let have a limiter with frequency limit: one connection per
minute.
Suppose following scenario:
1. A new connection is open at 7:01:00 for user M. It is the first
connection and so it should pass.
2. A new connection is open at 7:01:59 for user M. It should be rejected.
3. A new connection is open at 7:02:01 for user M. It should pass. But there
could be multiple limiters and the result depends on how fast can the second
connection revert its changes.
A shared common lock is required that ensures the atomicity of the
manipulation with counters. The lock has to ensure that the third connection is
waiting till the second finishes its work.
The simple iteration is not a feasible solution. If we fixed all mentioned
issues then the solution would become complicated and cluttered and the limiter
would need a new methods "deregister" and "revert". The solution requires also
significant changes in the exiting code of AMQPConnection_1_0 and other classes.
**My solution:**
The 'append' method creates a new chained limiter without any impact on the
input limiters. It does not have any side effect and so the 'append' operation
can be repeated.
`ConnectionLimiter chainedLimiter = limiterA.append(limiterB). // The
operation does not change neither limiterA nor limiterB. The chainedLimiter is
a new limiter that contains limiterA and limiterB.`
The chained limiter ensures proper locking and avoids the concurrency
issues. Every user has his own independent lock. Hence, multiple connections
from different users can be processed in parallel. The append operation does
not have any impact on input limiters. Therefore, it is possible to build a new
chained limiter while the old limiter stays untouched and in use. It means easy
update when the configuration is changed.
The chained limiter takes care of the consistency issue. The registration is
splitted into three steps:
1. The user's lock is requested at first.
2. All counters are check.
3. If only all checks pass then all counters will be incremented.
There is no need for a revert method. The register method returns a
connection slot, the connection slot points to a counter object. It remembers
which counter should be decremented when the connection slot is released.
The connection slots can be chained and so the chained limiter returns a
chained connection slot. The connection slot remembers correct counters
regardless of the subsequent configuration change. Therefore, it effectively
solves problems caused by any configuration change and neither 'deregister' nor
'revert' method of the limiter are needed.
The cached limiter returns the same connection slot when the same connection
is registered multiple times, AMQPConnection_1_0 can try to register itself
multiple times. Cached limiter associates the connection with a slot and the
following condition is held:
`cachedLimiter.register(connection) == cachedLimiter.register(connection)`
The connection slot produced with cached limiter is released only once
regardless how many times the method 'free' is executed.
All user limiting logic is focused in implementations of ConnectionLimiter
interface and limiters take care of synchronization, consistency, configuration
changes.
**BlockEveryone limiter:**
Let wrap 'BlockEveryone' limiter in cache decorator that utilizes a map as
the internal cache. But the cache remains empty forever because the register
method of 'BlockEveryone' limiter always throws an exception. There is not any
difference in acting of wrapped and unwrapped 'BlockEveryone' limiter.
'BlockEveryone' limiter acts as cached limiter, hence it is a cached limiter.
**NoLimits limiter:**
Let wrap 'NoLimits' limiter in cache decorator that utilizes a map as the
internal cache. But the cache will contain the same response FreeSlot.INSTANCE
for every input and so it is useless. The wrapped and unwrapped 'NoLimits'
limiter returns the same response for every input.
Hence, 'NoLimits' limiter acts as cached limiter and so it is a cached
limiter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
