Opened 6 years ago

Closed 4 years ago

#11894 closed Patch - Bug Fix (Fixed)

Patch: Prevent Services API from telling clients to allow Keep-Alive

Reported by: Bill Meek <keemllib@…> Owned by:
Priority: minor Milestone: 0.28
Component: MythTV - Services API - Backend Version: Master Head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

If a Services API client includes the "Conversation" "Keep-Alive" header in a request, the response will contain the same header information.

However, the Services API does not support persistent connections.

This 'patch' returns "Close" if a client requests "Keep-Alive" which will prevent TCP RST (resets) when clients try to reuse a connection.

Not a problem for clients themselves, which can put "Close" in their headers. However, libraries they have no control over, should receive a "Close" if "Keep-Alive" is sent.

Tested on: v0.28-pre-325-g326b686. Only with a Services API client, not any other users of this code.

diff --git a/mythtv/libs/libmythupnp/httprequest.cpp b/mythtv/libs/libmythupnp/httprequest.cpp
index 2758272..4afbb86 100644
--- a/mythtv/libs/libmythupnp/httprequest.cpp
+++ b/mythtv/libs/libmythupnp/httprequest.cpp
@@ -1028,7 +1028,7 @@ bool HTTPRequest::GetKeepAlive()
     if ( sConnection == "close" )
         bKeepAlive = false;
     else if (sConnection == "keep-alive")
-        bKeepAlive = true;
+        bKeepAlive = false;
 
    return bKeepAlive;
 }

Attachments (2)

MAF.with.keep-alive.protocol (6.3 KB) - added by Bill Meek <keemllib@…> 6 years ago.
Capture with the existing Keep-Alive (failure)
MAF.with.close.protocol (6.4 KB) - added by Bill Meek <keemllib@…> 6 years ago.
Capture with client modified to send Connection Close (same result as using the patch)

Download all attachments as: .zip

Change History (6)

comment:1 Changed 6 years ago by dblain

Resolution: Won't Fix
Status: newclosed

The service API and for that matter the http server, since it shares the same code, does support persistent connections. The connection is only closed if Keep-alive isn't set, or there is a problem with the connection.

I'm closing this ticket. If you can provide a use case that shows an actual problem, then re-open the ticket with the details on how to recreate it.

comment:2 in reply to:  1 Changed 6 years ago by Bill Meek <keemllib@…>

Resolution: Won't Fix
Status: closednew

Replying to dblain:

Thanks for the response.

The service API and for that matter the http server, since it shares the same code, does support persistent connections. The connection is only closed if Keep-alive isn't set, or there is a problem with the connection.

Sorry if I'm repeating things you already know. I'd suggest the second sentence here:

http://tools.ietf.org/html/rfc2616#section-8.1.2.1

says MythTV should return Close in it's responses because it knows it can't do persistent connections. No fair letting the client think that MythTV can.

I'm closing this ticket. If you can provide a use case that shows an actual problem, then re-open the ticket with the details on how to recreate it.

The use case I'm testing is for the MythTV Android Frontend project. I've attached two protocol captures. One with the existing HTTP header where the client asks MythTV to use Keep-Alive (and MythTV says OK by returning Keep-Alive.) The second is with client side code modified to always ask for "Connection" "Close". The result is the same as the suggest patch. It lets the client ask for Keep-Alive and MythTV says no, Close. I believe the patch is the correct solution. Future versions of MythTV may allow persistent connections, per discussions seen on the Developer's channel, and the clients needn't change.

To be honest, this is a low runner problem.

Changed 6 years ago by Bill Meek <keemllib@…>

Capture with the existing Keep-Alive (failure)

Changed 6 years ago by Bill Meek <keemllib@…>

Attachment: MAF.with.close.protocol added

Capture with client modified to send Connection Close (same result as using the patch)

comment:3 Changed 4 years ago by Bill Meek <keemllib@…>

Please close.

Retested on v0.28-pre-3180-gca36acd and see no problems.

The fix appears to be in: e3940dc.

comment:4 Changed 4 years ago by Karl Egly

Milestone: unknown0.28
Resolution: Fixed
Status: newclosed

Thank you for getting back at this ticket Bill. I am resolving it as fixed in master.

Note: See TracTickets for help on using tickets.