First feeback about "clean connection API".

[sbernard31]

I begin to use the new DTLSConnector API to remove connections. (startForEach, startTerminateConnectionsForPrincipal, ...)

For now, I try to implement an optional feature where connection is removed when Leshan SecurityInfo is removed from Leshan SecurityStore. (Maybe one day, I will really works on connection/session lifetime :sweat_smile: ...)

1. If you want to remove connection without using startDropConnectionsForPrincipal and startTerminateConnectionsForPrincipal there is not easy way because we haven't easy access to connectionStore to call connectionStore.remove(connection) in startForEach.

2. Implementing this : I have some concern about a potential race condition. Imagine :

   • User replaces a SecurityInfo. (e.g. same identity, different key)
   • We start a "removing task" for connection using the given identity.
   • At the same time device/client reconnects itself with the same identity with the new key.
   • the removing task is executed and the new connection is removed.

About 2, I'm not sure this is a real case issue, my first idea to avoid that would be to try to only remove "old" connection by checking DTLSSession.getLastHandshakeTime(). If this is a good idea maybe this could make sense to also do this check in startTerminateConnectionsForPrincipal ?

[boaks]

1.

May idea was more a "callback interface", which reports to remove the connection or not. In my opinion that would keep the public API smaller and doesn't expose the connection store just for remove.

Currently the return value is used to stop the iteration (startForEach) and to remove the connection (startTerminateConnectionsForPrincipal(Predicate<Principal>)). Maybe a startTerminateConnectionsForSession(Predicate<DTLSSession>) or startTerminateConnections(Predicate<Connection>) helps more. Maybe it's also a good idea to extend the return value into an enum { NEXT, DROP, STOP }. So, if you feel to have a good use-case, go for it.

About 2:

For me this looks like the old "secret" got "published". For that rare case, I would accept the "race" and shutdown the connection in doubts. The device will be able to connect again. This will happen only rarely, so I don't see a real issue there. Just for changing the secret, it's not required to remove the old connection, or? And, yes, maybe the session or credentials requires more data to do that more smarter. But that depends on the use-case, which I assume to be reported and then the extensions being contributed.

[sbernard31]

Just for changing the secret, it's not required to remove the old connection, or?

Not really but this it seems to be the behavior expected (see https://github.com/eclipse/leshan/issues/162) and I can understand that. You remove or change the credential you expect client is not able to connect. (but maybe this is an incorrect expectation)

In my opinion that would keep the public API smaller and doesn't expose the connection store just for remove.

I'm not so sure about this. As to me ConnectionStore is already exposed:

protected DTLSConnector(final DtlsConnectorConfig configuration, final ResumptionSupportingConnectionStore connectionStore) 

With some line of code I can access to connectionStore but this is not so easy. And for now, I'm not sure to see the benefits of adding this new function. I would feel more free with a store access so I can remove 1 or several items in 1 loop, I can remove connection or session or both at same time.

[boaks]

I would feel more free with a store access

I would be afraid, that the "serialized execution" will not be obeyed. The iterating approach with the return value was intended to ensure that.

I'm not so sure about this. As to me ConnectionStore is already exposed:

Yep. And if someone corrupts the serial execution it will not work. For me the intention to pass that in is, to provided a other implementation rather then to use the store from extern.

[sbernard31]

Ok I get your point, a solution could be to warn user in documentation about which method MUST be called in the "serialized execution". (In all case this kind of documentation make sense)

Another one would be to have access to the store in the callback ?

something like :

     boolean accept(ConnectionStore store, V value);

[boaks]

I don't get it! Why should the return value not be used to define, if the connection or what ever should be removed?

startTerminateConnectionsForPrincipal
...
public boolean accept(Connection connection) {
    Principal peer = null;
    SessionTicket ticket = connection.getSessionTicket();
    if (ticket != null) {
        peer = ticket.getClientIdentity();
    } else {
        DTLSSession session = connection.getSession();
        if (session != null) {
            peer = session.getPeerIdentity();
        }
    }
    if (peer != null && principalHandler.accept(peer)) {
        connectionStore.remove(connection, true);
    }
    return false;
}

Something similar to that example implementation.

[sbernard31]

I don't get it! Why should the return value not be used to define, if the connection or what ever should be removed?

You said we have an enum with NEXT, STOP, DROP

It seems to be too strict. How can I drop but continue to iterate ? (eg. to remove several connection in 1 loop) How can I choose to not remove the session but just the connection ? So I proposed something which seems more flexible to me, trying to taking into account your remark about "serialized execution".

[boaks]

Maybe it's also a good idea to extend the return value into an enum { NEXT, DROP, STOP }.

That was an idea to extend the current mechanism. And the idea could be also more extended. e.g. return logical flags b001 = next/stop b010 = drop connection, b100 drop session ( and the combinations).

[sbernard31]

It can but it's hard to me to understand why it seems so important to avoid users to directly use their connection store.