Bug 31033

Summary: SIP2 configuration does not correctly handle multiple simultaneous connections by default
Product: Koha Reporter: Mark Alexander <marka>
Component: SIP2Assignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: dcook, kyle, lucas, martin.renvoize, tomascohen, victor
Version: MainKeywords: release-notes-needed
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00,22.05.06,21.11.12
Attachments: Bug 31033: Explicitly define max child processes for SIP server
Bug 31033: Explicitly define max child processes for SIP server
Bug 31033: Explicitly define max child processes for SIP server

Description Mark Alexander 2022-06-23 19:34:58 UTC
If two connections are made to the SIP2 server simultaneously, the second connection does not receive any replies until the first connection is closed.  Here is how I reproduced the problem:

In terminal #1, use 'telnet koha.ourlibrary.com 6001' to connect to
the SIP2 server.  Send a 9300CN message to log into SIP2.  The SIP2
server sends back the expected 941 response.

In terminal #2, use telnet in the same way to connect
to the SIP2 server.  Then try sending the 9300CN message to log in.
The SIP2 does not send any response.      

In terminal #1, close the telnet connection.  Now terminal #2
receives its expected 941 message.

This problem was discovered by Lyrasis, a content provider that we are trying to integrate with our library's Koha installation.
Comment 1 David Cook 2022-06-24 01:21:00 UTC
That's quite interesting.

Using koha-testing-docker, I ran "telnet localhost 6001" from two different terminals. In a third terminal, I ran the following "ss -l -n -t" and I can see that the SIP process listening on 127.0.0.1:6001 has 1 connection in its TCP "listen" backlog. 

While the telnet client says it's connected, that just mean it's connected to the server's network stack. Only one connection has actually been accepted by the application. (This would probably be clear from your prod syslog as well.)

Looking at the "server-params" in the SIPconfig.xml file, it looks like min_servers = 1 and min_spare_servers = 0. In theory, 0 should be fine because that should just means 0 spare servers...

When I change min_spare_servers to = 1, the forking seems to work correctly. If there are no current connections, then there is only 1 child process. If there are current connections, then it keeps 1 extra connection around. 

I'd say it's a bug/quirk in Net::Server::PreFork which powers the SIP server. 

Since it looks like the SIP server has accidentally been using a max of 1 child process, we should probably set some limits on maximums since the default max server is 50. The default max spare servers is 10...
Comment 2 David Cook 2022-06-24 01:22:43 UTC
Mark, your problem is solvable by changing your SIPconfig.xml file, but I think this bug is worth keeping open for now, as maybe we should change the defaults for the Koha SIP server, or at least add some comments to make it clear how it works.
Comment 3 David Cook 2022-06-24 01:42:18 UTC
It looks like Net::Server::Prefork subclasses Net::Server::PreForkSimple which subclasses Net::Server and I'm not sure that the "max_server" configuration item actually gets passed along correctly... 

So I'm going to add a default of "1" for Koha since that seems to be what's used in practice... 

In your case, I'd say just add "max_servers='2'" or whatever you need to support your situation.
Comment 4 David Cook 2022-06-24 01:47:36 UTC
Created attachment 136470 [details] [review]
Bug 31033: Explicitly define max child processes for SIP server

By default, the SIP server appears to only use 1 child process for
responding to SIP connections.

This change makes this explicit in the configuration, which should
make it so that people who need more than 1 simultaneous SIP connection
can know to just increase the value for the "max_servers" parameter
in the SIPconfig.xml file.

Test plan:
1. Add "max_servers='1'" to your SIP configuration file
2. koha-sip --restart kohadev
3. Open 3 terminals
4. Run "telnet localhost 6001" on 2 terminals
5. On the 3rd terminal, run the following:
ss -l -n -t
ps -efww | grep "sip"
6. Note that there are 2 processes called
kohadev-koha-sip: perl /kohadevbox/koha/C4/SIP/SIPServer.pm /etc/koha/sites/kohadev/SIPconfig.xml
One of these processes is the parent of the other
7. The Recv-Q in the "ss" output should show 1
(This means that 1 of your telnet connections is in the server's TCP backlog)
8. Celebrate as the configuration works as expected
Comment 5 Mark Alexander 2022-06-24 03:00:39 UTC
Thanks.  Setting max_servers to '2' works for my use case with two simultaneous connections.
Comment 6 David Cook 2022-06-24 03:03:47 UTC
(In reply to Mark Alexander from comment #5)
> Thanks.  Setting max_servers to '2' works for my use case with two
> simultaneous connections.

Great. Glad I was able to help. I enjoy networking issues heh.
Comment 7 Kyle M Hall 2022-08-11 11:45:53 UTC
Created attachment 139020 [details] [review]
Bug 31033: Explicitly define max child processes for SIP server

By default, the SIP server appears to only use 1 child process for
responding to SIP connections.

This change makes this explicit in the configuration, which should
make it so that people who need more than 1 simultaneous SIP connection
can know to just increase the value for the "max_servers" parameter
in the SIPconfig.xml file.

Test plan:
1. Add "max_servers='1'" to your SIP configuration file
2. koha-sip --restart kohadev
3. Open 3 terminals
4. Run "telnet localhost 6001" on 2 terminals
5. On the 3rd terminal, run the following:
ss -l -n -t
ps -efww | grep "sip"
6. Note that there are 2 processes called
kohadev-koha-sip: perl /kohadevbox/koha/C4/SIP/SIPServer.pm /etc/koha/sites/kohadev/SIPconfig.xml
One of these processes is the parent of the other
7. The Recv-Q in the "ss" output should show 1
(This means that 1 of your telnet connections is in the server's TCP backlog)
8. Celebrate as the configuration works as expected

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Martin Renvoize 2022-08-23 15:06:38 UTC
Created attachment 139687 [details] [review]
Bug 31033: Explicitly define max child processes for SIP server

By default, the SIP server appears to only use 1 child process for
responding to SIP connections.

This change makes this explicit in the configuration, which should
make it so that people who need more than 1 simultaneous SIP connection
can know to just increase the value for the "max_servers" parameter
in the SIPconfig.xml file.

Test plan:
1. Add "max_servers='1'" to your SIP configuration file
2. koha-sip --restart kohadev
3. Open 3 terminals
4. Run "telnet localhost 6001" on 2 terminals
5. On the 3rd terminal, run the following:
ss -l -n -t
ps -efww | grep "sip"
6. Note that there are 2 processes called
kohadev-koha-sip: perl /kohadevbox/koha/C4/SIP/SIPServer.pm /etc/koha/sites/kohadev/SIPconfig.xml
One of these processes is the parent of the other
7. The Recv-Q in the "ss" output should show 1
(This means that 1 of your telnet connections is in the server's TCP backlog)
8. Celebrate as the configuration works as expected

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2022-08-23 15:07:00 UTC
Trivial improvement, works.. Passing QA
Comment 10 Tomás Cohen Arazi 2022-08-25 11:55:10 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 11 Kyle M Hall 2022-08-29 18:15:23 UTC
I have no idea what's going on, but in production this is breaking SIP!
No errors in the logs.
Is anyone else experiencing this?
Comment 12 Kyle M Hall 2022-08-29 18:40:15 UTC
(In reply to Kyle M Hall from comment #11)
> I have no idea what's going on, but in production this is breaking SIP!
> No errors in the logs.
> Is anyone else experiencing this?

Ignore this^ I had set max lower than min. That was my bad.
Comment 13 David Cook 2022-09-19 06:32:35 UTC
(In reply to Kyle M Hall from comment #11)
> I have no idea what's going on, but in production this is breaking SIP!
> No errors in the logs.
> Is anyone else experiencing this?

I had an issue with SIP today although it wasn't the max_servers. It was that min_spare_servers was set to 0. 

(In reply to David Cook from comment #1)
> Looking at the "server-params" in the SIPconfig.xml file, it looks like
> min_servers = 1 and min_spare_servers = 0. In theory, 0 should be fine
> because that should just means 0 spare servers...
> 
> When I change min_spare_servers to = 1, the forking seems to work correctly.
> If there are no current connections, then there is only 1 child process. If
> there are current connections, then it keeps 1 extra connection around. 
> 
> I'd say it's a bug/quirk in Net::Server::PreFork which powers the SIP
> server. 
> 
> Since it looks like the SIP server has accidentally been using a max of 1
> child process, we should probably set some limits on maximums since the
> default max server is 50. The default max spare servers is 10...

The real issue is min_spare_servers...

I think my intention here was that min_servers='1' and min_spare_servers='0' implies a max_servers='1'.

But if you change max_servers='10', it won't make a difference until you change "min-spare_servers" to at least 1.
Comment 14 Katrin Fischer 2022-09-19 09:06:02 UTC
I've rephrased the bug title a bit as I had already someone asking if Koha can't currently support multiple devices from reading it - just a configuration issue!
Comment 15 Tomás Cohen Arazi 2022-09-19 11:11:41 UTC
Can you please document this bugfix correctly for the release notes?
Comment 16 David Cook 2022-09-19 23:06:01 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> Can you please document this bugfix correctly for the release notes?

Well it's not a bugfix. It's just making the configuration more explicit, although perhaps not explicit enough. Perhaps we should be adding comments in the SIPconfig.xml to make it clearer on how to configure it.
Comment 17 David Cook 2022-09-19 23:07:02 UTC
Looking back at the commit message... I need to take another look at this in koha-testing-docker to double-check the wording...
Comment 18 Tomás Cohen Arazi 2022-09-20 00:09:45 UTC
Please add some release notes so people notice it. It is a good thing to advertise.
Comment 19 Lucas Gass 2022-10-14 22:17:17 UTC
Backported to 22.05.x for upcoming 22.05.06 release
Comment 20 Victor Grousset/tuxayo 2022-10-21 22:31:46 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.
Comment 21 Arthur Suzuki 2022-10-22 22:08:33 UTC
Thanks!

Pushed to 21.11 for 21.11.12