patchConcurrent Versions System - Patches: patch #4992, Client/Server connection timeout

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #4992: Client/Server connection timeout

Submitter:  Derek Robert Price <dprice>
Submitted:  Fri 24 Mar 2006 07:50:47 PM UTC
   
 
Category:  Feature (patch attached) Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
Fixed Release:  None Fixed Feature Release:  None

Jump to the original submission

Wed 05 Apr 2006 02:46:18 PM UTC, comment #6: 

Since very large timeouts may make sense, I've committed a change to accept `d', `h', `m', & `s' units (days, hours, minuts, & seconds, respectively) as arguments to --timeout and updated the documentation accordingly, including a warning about short server timeouts.

What about a default like 24h, or even 30m?  That seems like plenty of time to leave a connection open and there shouldn't be much more CPU cost to run this code.

Derek Robert Price <dprice>
Group administrator
Wed 05 Apr 2006 02:22:37 PM UTC, comment #5: 

Larry Jones writes:

Correct me if I'm wrong, but doesn't commit also have to wait for the client's editor session for the log message?  If so, I'd say something like 10 minutes is the absolute minimum we should recommend.  And I'd strongly support keeping 0 as the default -- KEEPALIVE is a much better solution to the problem (on systems where it actually does something instead of just being a placebo).

Derek Robert Price <dprice>
Group administrator
Tue 28 Mar 2006 03:33:09 PM UTC, comment #4: 

I've committed this patch plus doc and a test, with a default timeout value of 0.  I'm not closing this issue until we settle the default timeout discussion.

Derek Robert Price <dprice>
Group administrator
Tue 28 Mar 2006 01:04:37 PM UTC, comment #3: 

It's possible CVSLCKSLEEP + something bigger than 1 might make sense with a heavily loaded server.  Perhaps with a commit, which I dare say might have to wait for a lock, the check pass might run for longer than a second before getting to the second (commit) pass and returning any data, causing an undesirable timeout with this default.

Maybe CVSLCKSLEEP + 30 would be a more reasonable default?  It seems somewhat reasonable to expect any user dealing with a server that would take longer than 30 seconds to run the commit check pass to know they should lengthen the timeout.  Anything longer seems unnecessary and tedious for the user (keeping in mind that this will actually take a total of 60 seconds to timeout).

If we're wrong, it will be easy enough to fix after the first few bug reports.

Derek Robert Price <dprice>
Group administrator
Tue 28 Mar 2006 12:26:13 AM UTC, comment #2: 

The present default in main.c:

>+long connection_timeout = 0;


will mean that the functionality
of CVS waiting forever not change
by default. I therefore consider
this to be a fairly safe patch at
present.

I would support a non-zero default
to enable timeouts to be used, and
the CVSLCKSLEEP + 1 value seems like
it could be okay, but even a longer
delay as a default would still make
sense to me given that the current
default behavior is to wait forever.

Mark D. Baushke <mdb>
Group administrator
Mon 27 Mar 2006 01:32:17 PM UTC, comment #1: 

I'm attaching a revised patch based on private review by Mark Baushke and the subsequent discussion.

We are currently discussing a useful default for the connection timeout value.  I am currently leaning towards CVSLCKSLEEP + 1 so that clients with connections to the server with subsecond lag do not timeout while the server is waiting to get a lock, at least not  by default.

Derek Robert Price <dprice>
Group administrator
Fri 24 Mar 2006 07:50:47 PM UTC, original submission:  

Any thoughts on the attached patch to feature, other than the fact that I haven't added test cases and doc yet?  It adds a connection timeout to the client or server via a command line option.

The original impetus for this feature was a bunch of hung CVS servers on Solaris that appear to be hung either in the read to or write from net routines.  Since some of these servers were hung for weeks and the reporter was fairly certain that the client connections were gone, it is fairly obvious that SO_KEEPALIVE wasn't working as advertised on this system to return an error when the connection breaks.

This was the fix I came up with.  It has the additional advantages of noticing broken connections after a configurable amount of time (a working SO_KEEPALIVE is defined such that it can take up to two hours to notice a broken connection) and being a line of defense against simple DOS attacks that might open multiple connections to a CVS server without sending data across them in an attempt to create many servers and fill up the server's available memory.

Once you have applied this patch, a simple test it to run `cvs --timeout 5 server' and see the error message output after 5 seconds.

I may have to add a bit of timeout code to the proxy server select()s as well.  If this timeout feature is considered acceptable, I will add that with tests and doc before committing the final version.

Derek Robert Price <dprice>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #9690:  ccvs-timeout3.diff added by dprice (13KiB - application/octet-stream - Revised patch based on private review by Mark Baushke.)
file #9685:  ccvs-timeout.diff added by dprice (8KiB - application/octet-stream - The patch to add a configurable timeout to client/server connections.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

 

Follow 2 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2006-03-27 dprice Attached File- Added ccvs-timeout3.diff, #6076
2006-03-24 dprice Attached File- Added ccvs-timeout.diff, #6071

Back to the top

Powered by Savane 3.13-f8d8.
Corresponding source code