-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Increase unauthenticated session pool, fixes #10493 #10652
Conversation
PR #10652: Size comparison from 4c7f664 to 8d7b98c 5 builds (for p6, qpg, telink)
Increases above 1.0% from 4c7f664 to 8d7b98c:
17 builds (for efr32, k32w, linux, mbed)
12 builds (for esp32, nrfconnect)
|
Size increase report for "gn_qpg-example-build" from 4c7f664
Full report output
|
Size increase report for "esp32-example-build" from 4c7f664
Full report output
|
Size increase report for "nrfconnect-example-build" from 4c7f664
Full report output
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should unauthenticated sessions not expire in time? #10493 seems to complain that if running several commisioning (4 of them?) eventually we run out of resources.
I am wondering if this PR just increases the time it takes for things to fail. Is this pool increase supposed to be a final solution or a temporary change until we figure out how to expire unauthenticated connections? In my mind, we should just LRU unauthenticated connections and have the pool quite small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to ask the same question @andy31415 asked: this seems like it's just working around the fact that the TE is trying to create 4-5 authenticated connections one after another, not actually fixing things to work when we have had more unauthenticated connection attempts in our lifetime than the pool size.
The rule we have is in fact LRU eviction. However, the session manager has no way to forcibly evict when reference counts are non-zero. To do so, the session manager would need to understand (or be able to learn) what resources are associated with its sessions to call on-failed type methods on all associated things. It is my view that the inability to do this is ultimately the problem. Since we effectively have a multi-singleton architecture with the single session manager instance and single exchange manager instance, it would probably make most sense for the exchange manager to expose a method to forcibly cancel all exchanges associated with a session. I do not know how difficult this would be to implement. |
It is LRU controlled, the session should be released when PASE/CASE session is completed. I'll check why the session is not being freed. |
The nuance to my point was that it may still be too expensive to wait for PASE/CASE session timeout or completion. If so., the session manager would need to have some mechanism for forcible eviction. However, if even happy-path cleanup is not occurring, then yes, that would be a more immediate problem. |
Problem
#10493
Change overview
Increase the pool size of unauthenticated session
Testing
Verified by unit-tests.