Bug 13807

Summary: SIPServer Input loop not checking for closed connections reliably
Product: Koha Reporter: Colin Campbell <colin.campbell>
Component: SIP2Assignee: Colin Campbell <colin.campbell>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: colin.campbell, gitbot, julian.maurice, kyle, m.de.rooy, mtj
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15006    
Attachments: Proposed Patch
[SIGNED-OFF] Bug 13807 Rework main input loop in SIPServer
Bug 13807 Rework main input loop in SIPServer

Description Colin Campbell 2015-03-06 15:31:57 UTC
Debugging some sip connections found that a server could get stuck with a partially unread buffer when the remote closed the connection. This could mean it was also not responding to signals to shut down. Reworked the main loop to be a bit more careful about checking the connection status. Moved it from the Sip.pm module into the server where it seemed to belong more logically.

Have been running this with some other modifications in production sites for some time and appears to have enhanced reliability and ability to control the server.
Comment 1 Colin Campbell 2015-03-06 15:36:10 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2016-06-07 05:19:20 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2016-06-24 17:36:27 UTC
Colin, how does this patch work with bug 15006? At the moment they seem to have merge conflicts.
Comment 4 Kyle M Hall 2016-06-24 17:43:46 UTC
Created attachment 52854 [details] [review]
Bug 13807 Rework main input loop in SIPServer

Debugging various problems in SIPServer and control of it, found it
could loop on unread buffers (e.g. the LF of a CRLF if it was only
expecting CR) making it unresponsive to signals.
Reworked the input loop with an eye to removing unnecessary whiles
and replacing the while(1) by a while( connection valid)
Enhanced the timeout code by wapping in an eval.
Moved the logic from SIP_read_packet into the server itself
Hopefully this makes the already baroque code easier to navigate
and it did seem the server was the logical place for this
Removed no longer iused SIP_read_packet from Sip.pm

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Kyle M Hall 2016-06-24 17:46:04 UTC
Passing QA provisionally, we should deal with the interaction with bug 13807 before anything is pushed.
Comment 6 Marcel de Rooy 2016-07-01 06:52:10 UTC
(In reply to Kyle M Hall from comment #5)
> Passing QA provisionally, we should deal with the interaction with bug 13807
> before anything is pushed.
13807->15006
I rebased 15006 on top of this one. So it can go first..
Comment 7 Colin Campbell 2016-07-01 14:49:43 UTC
(In reply to Marcel de Rooy from comment #6)
> (In reply to Kyle M Hall from comment #5)
> > Passing QA provisionally, we should deal with the interaction with bug 13807
> > before anything is pushed.
> 13807->15006
> I rebased 15006 on top of this one. So it can go first..

I'm trying to add to 15006 a solution to the log in bug I identified, I'll generated that on top of this
Comment 8 Kyle M Hall 2016-07-08 15:00:35 UTC
Pushed to master for 16.11, thanks Colin!
Comment 9 Julian Maurice 2016-08-17 08:48:13 UTC
This has been pushed to 16.05.x (August 1st), will be in 16.05.02
Comment 10 Julian Maurice 2016-08-17 08:48:50 UTC
Patch pushed to 3.22.x, will be in 3.22.10