Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LeastRecentlyUsedCache behavior. #707

Closed
sbernard31 opened this issue Jul 26, 2018 · 8 comments
Closed

LeastRecentlyUsedCache behavior. #707

sbernard31 opened this issue Jul 26, 2018 · 8 comments

Comments

@sbernard31
Copy link
Contributor

sbernard31 commented Jul 26, 2018

Currently, the LeastRecentlyUsedCache behave like this :

Javadoc:
An in-memory cache with a maximum capacity and support for evicting stale entries based on an LRU policy.
The cache keeps track of the values' last-access time automatically. Every time a value is read from or put to the store, the access-time is updated.
A value can be successfully added to the cache if any of the following conditions is met:
- The cache's remaining capacity is greater than zero.
- The cache contains at least one stale entry, i.e. an entry that has not been accessed for at least the cache's expiration threshold period. In such a case the least- recently accessed stale entry gets evicted from the cache to make place for the new value to be
added.
For the get :
If the cache contains the key but the value is stale the entry is removed from the cache.

Meaning
So this means that after threshold seconds of inactivity and even if the element is still in the store, it will be considered as absent.

Should we change this ?
In californium does it make sense to remove element on get ?
I mean why not using this data if we keep it in memory all this time.
Maybe we could consider threshold as a warranty that the entry can not be removed during this time but not as lifetime.

By the way, I believe the find method seems to not remove stale entries.

Current usage of LeastRecentlyUsedCache:

  • DTLS connection
  • Message ID
  • transparent blockwise transfer data

WDYT ?

@boaks
Copy link
Contributor

boaks commented Jul 26, 2018

In #617 the idea is to remove a even recently used DTLS session (based on a other timeout).
For me this is somehow "running in circles" :-).
The ideas seems to be triggered by issue
eclipse-leshan/leshan#542
but I'm not sure, what the "common expected" functionality should be for LRU-cache.

My guess is, a standard component was copied and adjusted for the special purpose (without adjusting the documentation) and the find was then added completely "out of scope" (it doesn't update the usage timestamp).
If the above change should really be implemented, I would prefer to have it moved into a own implementation with correct documentation.

@sbernard31
Copy link
Contributor Author

#617 is something else it is about a maximum session(connection?) lifetime.

Could we agree first about the behavior we would like then see how we could implement this ?

@boaks
Copy link
Contributor

boaks commented Jul 27, 2018

Let me split it into two parts:

For short term:
We had already a short discussion in #343 about the stale dtls sessions. I'm OK with a get(), which doesn't apply "expired". I have overseen that usage. When I was switching from currentTimeMillis to nanoTime, I got aware, that get() uses the expire-timespan, and put() the expire-timepoint for stale(). With 600s and currentTimeMillis, get() never expired. With the documentation in mind, which states to expire the get(), I "fixed" it. But I wasn't really aware, that this also violates #343. So I would prefer to either provide an additional get() or an parameter to select the behavior or to clone and move the implementation.

Long term:
I'm not really convinced, that a a short "stale time" used for stale dtls handshakes is a good solution.
Especially for eclipse-leshan/leshan#542 the pings will not update the last usage time and so, in shortage of cache entries, these elements will be removed. But I don't plan to invest time into that for the next months.

boaks pushed a commit to bosch-io/californium that referenced this issue Jul 31, 2018
Update find and iterator to "evict on read access" processing including
the update of the last-access time.
Setup the usage of the LR to explicit define the "evict on read access"
mode.
Fix issue eclipse-californium#707.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
boaks pushed a commit that referenced this issue Jul 31, 2018
Update find and iterator to "evict on read access" processing including
the update of the last-access time.
Setup the usage of the LR to explicit define the "evict on read access"
mode.
Fix issue #707.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
@sbernard31
Copy link
Contributor Author

Especially for eclipse-leshan/leshan#542 the pings will not update the last usage time and so, in shortage of cache entries, these elements will be removed.

I think the ping must not update the last usage time, as in the case of eclipse-leshan/leshan#542 the ping is not a valid DTLS message.

@boaks
Copy link
Contributor

boaks commented Aug 2, 2018

I think the ping must not update the last usage time

I didn't wrote, that the ping should update the last usage time. I just wrote, that with that behavior, the ping clients are in danger of being removed from the cache, if other clients are try to establish a DTLS session.

The pitfall here is, that even trying clients (which may not have the proper credentials) would evict valid ping clients. That's why I think, stale DTLS handshakes should be treated different from stale DTLS sessions.

@sbernard31
Copy link
Contributor Author

I didn't wrote, that the ping should update the last usage time. I just wrote, that with that behavior, the ping clients are in danger of being removed from the cache, if other clients are try to establish a DTLS session.

I meant that the keepalive behavior is "normal", and so the potential issue you expose about "valid connection eviction by state handshakes" is a general issue which is not really linked to keepalive.

I agree that a short stale time used for stale dtls handshakes is not really a good solution. As you probably want a threshold shorter for incomplete handshake than established connection...

For client initiated use case like LWM2M queue mode, I think this is pretty OK to have a short threshold but for always "connected" use case this is another issue...

Currently if you want to ensure that your connection will not be "stolen", you need to send data before the MAX_PEER_INACTIVITY_PERIOD. (like a coap ping or DTLS heartbeat if we implement it? )

even trying clients (which may not have the proper credentials) would evict valid ping clients.

I don't know if this is a real problem because :

  • if there is just some invalid clients which evict valid clients its not really an issue because those clients would be probably evicted few time later by other valid clients. (not optimal but no critical)
  • if there is a lot of clients, we face a kind of DoSS attack and this would not be the only issue you will face.

Maybe, connection not linked to established session should be store in another structure with shorter threshold ? So in case of Doss attack, you will probably not be able to connect more valid clients as all slot will be taken by attackers, but already connected client would be "safe".

@boaks
Copy link
Contributor

boaks commented Oct 3, 2018

@sbernard31

the changes, which caused the different behavior are reverted in the meantime.
Could you please consider to close this issue?

@sbernard31
Copy link
Contributor Author

Fixed in Cf 2.0.0-M12 (confirmed by @bernhard-seifert at eclipse-leshan/leshan#542 (comment))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants