Bug 13807 - SIPServer Input loop not checking for closed connections reliably
Summary: SIPServer Input loop not checking for closed connections reliably
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Colin Campbell
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15006
  Show dependency treegraph
 
Reported: 2015-03-06 15:31 UTC by Colin Campbell
Modified: 2017-12-07 22:16 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (9.60 KB, patch)
2015-03-06 15:36 UTC, Colin Campbell
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13807 Rework main input loop in SIPServer (9.64 KB, patch)
2016-06-07 05:19 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 13807 Rework main input loop in SIPServer (9.70 KB, patch)
2016-06-24 17:43 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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