[Asterisk-code-review] manager: be more aggressive about purging http sessions. (asterisk[master])

[Jaco Kroon]

Attention is currently required from: Joshua Colp.
Jaco Kroon has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18976 )

Change subject: manager: be more aggressive about purging http sessions.
......................................................................


Patch Set 2:

(3 comments)

Patchset:

PS2: 
Thank you Joshua, happy with the NACK, hope this clarifies, but would appreciate a bit of direction guidance here before proceeding further.


File include/asterisk/tcptls.h:

https://gerrit.asterisk.org/c/asterisk/+/18976/comment/c51e84bb_42e7b8bb 
PS2, Line 141: 	void (*periodic_fn)(struct ast_tcptls_session_args *);/*!< something we may want to run before after select on the accept socket */
> Why this change to the API? Everything else uses void * and then casts it in the callback.
Sure, I can do that (originally did), but this is safer type-wise.  And there are no other users of this in-tree so "all consumers has been updated".

The other option is to rip out this from the API entirely, but that should happen via deprecation, and rather use a task processes inside the manager module.

This whole periodic_fn really was just a "cheap" way to get a thread to periodically perform the purging on.  Also, after every purge, we can then actually calculate the correct time to sleep (ie, find the time to the next purge that needs to happen, that way we can potentially sleep up to httptimeout seconds at a time (if a new session is created right now it will get purged earliest httptimeout seconds from now, otherwise we can calculate how long to sleap based on the session that's closest to being timed out).  This is a much more complex change, but possibly "the right thing to do".  If you'd prefer that approach, how can I mark the use of periodic_fn as deprecated other than using a WARNING log?  Does it make sense to deprecate this?

Further towards this, this timer will only work if native AMI socket is enabled, not sure if it's possible to configure to NOT EVEN HAVE the plaintext socket on which this is applied, it could be, in which case the purge process won't even run at all.

Happy to take direction here.


File main/manager.c:

https://gerrit.asterisk.org/c/asterisk/+/18976/comment/d94fbc55_b5a163b0 
PS2, Line 8643: 	if (purge_sessions(1) == 1) {
> Document why it's behaving this way
Ack



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18976
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I4117ae25acc9c92bd1d3cfa73d7d334f5c3480c2
Gerrit-Change-Number: 18976
Gerrit-PatchSet: 2
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Tue, 16 Aug 2022 20:30:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220816/9d02628a/attachment.html>