Bug 220123 - [api][dstore] Configurable timeout on irresponsiveness
Summary: [api][dstore] Configurable timeout on irresponsiveness
Status: VERIFIED FIXED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 3.0   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 3.0 M6   Edit
Assignee: David McKnight CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks:
 
Reported: 2008-02-24 20:03 EST by Masao Nishimoto CLA
Modified: 2008-03-11 22:13 EDT (History)
0 users

See Also:


Attachments
patch providing configurable keepalive timeout values (31.18 KB, patch)
2008-02-27 16:36 EST, David McKnight CLA
no flags Details | Diff
patch for keepalive with server preferences separate from client (33.76 KB, patch)
2008-02-28 11:00 EST, David McKnight CLA
no flags Details | Diff
Patch for reving up dstore version from 2.0.100 to 2.1.0 (6.51 KB, patch)
2008-02-28 14:07 EST, Martin Oberhuber CLA
no flags Details | Diff
patch to handle case where IO_SOCKET_READ_TIMEOUT < KEEP_ALIVE_RESPONSE_TIMEOUT (4.14 KB, patch)
2008-03-06 14:01 EST, David McKnight CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Masao Nishimoto CLA 2008-02-24 20:03:07 EST
When a client has gone without disconnecting from a server, the server process terminates after some period of time.  The requirement is to make this timeout value configurable.
Comment 1 Martin Oberhuber CLA 2008-02-25 04:33:52 EST
Makes only sense for dstore, where we can control the server.

I'm wondering what the use-case is for configuring a long timeout? Would you expect that the client can re-connect automatically? Under what cirumstances could that happen?

I assume that in order to support the full story, the client would need some changes as well e.g. in order to support reconnect.
Comment 2 Masao Nishimoto CLA 2008-02-25 19:44:13 EST
This is a requirement for dstore server only, and for configuring a short timeout.

On a system where access to a file is serialized by locking the file, if a client has gone without unlocking files that the client has locks, the files need to be released by the server.  The timeout value defines how long it takes for the server to detect that the client has gone, and thus be able to release files locked by the client.
Comment 3 David McKnight CLA 2008-02-27 16:36:59 EST
Created attachment 90934 [details]
patch providing configurable keepalive timeout values

This patch provides configurable keepalive timeout values.  The values are stored as preferences and, on connect, they are applied to both the client XMLparser as well as the server's XMLparser. 

Keepalive requests can be initiated from both the client and the server when the socket appears idle.

The IO_SOCKET_READ_TIMEOUT determines how long to wait for new input on the socket before sending a keepalive request.

The KEEPALIVE_RESPONSE_TIMEOUT determines how long to wait for a response once a keepalive request has been made.

The DataStore preference page allows users to specify these values.

Please comment with any concerns here.
Comment 4 Masao Nishimoto CLA 2008-02-27 19:36:30 EST
The timeout values of one client affect all other clients.  e.g. If one client has longer timeout value and has gone without normal disconnect, the resources locked by the server on behalf of the client will be unusable for longer from any clients.  I prefer the server has its own timeout values, so that a system administrator can control them centrally.
Comment 5 David McKnight CLA 2008-02-28 10:06:30 EST
(In reply to comment #4)
> The timeout values of one client affect all other clients.  e.g. If one client
> has longer timeout value and has gone without normal disconnect, the resources
> locked by the server on behalf of the client will be unusable for longer from
> any clients.  I prefer the server has its own timeout values, so that a system
> administrator can control them centrally.

Okay.  Perhaps what I'll do is have the preferences only apply to the client, while have the server pick up the preference from a config file. 
Comment 6 David McKnight CLA 2008-02-28 11:00:09 EST
Created attachment 91016 [details]
patch for keepalive with server preferences separate from client

I've changed the code so that the client preferences for keepalive only apply to the client.  The server gets it's keepalive preferences from java properties.  The following can be passed as VM arguments when starting the server:

-DDSTORE_KEEPALIVE_ENABLED=<"true"|"false">
-DDSTORE_KEEPALIVE_RESPONSE_TIMEOUT=<value> 
-DDSTORE_IO_SOCKET_READ_TIMEOUT=<value>


Does this help?
Comment 7 David McKnight CLA 2008-02-28 11:15:28 EST
I'm marking this as api since we introduce the following APIS:

In XMLparser, I've added the following:

	/**
	 * Set whether to enable keepalive
	 * @param enable
	 */
	public void setEnableKeepalive(boolean enable){
		// if false, we ignore the keepalive stuff
		_isKeepAliveCompatible = enable;
	}
	
	/**
	 * Set the keepalive response timeout
	 * @param timeout the time to wait for a response after 
	 *        initiating a keepalivfe request
	 */
	public void setKeepaliveResponseTimeout(int timeout){
		// the new value will be picked up on the next readLine() call
		KEEPALIVE_RESPONSE_TIMEOUT = timeout;
	}

	/**
	 * Set the socket read timeout
	 * @param timeout the time to wait before initiating a keepalive request
	 */
	public void setIOSocketReadTimeout(int timeout){
		// the new value will be picked up on the next readLine() call
		IO_SOCKET_READ_TIMEOUT = timeout; 		
	}


A new interface is added so that classes on the client and server side can be notified of any changes to the DataStore preferences:

/**
 * Classes that implement this and add themselves to the DataStore preference listeners
 * get called each time a preference is changed.
 */
public interface IDataStorePreferenceListener {
	
	/**
	 * A DataStore preference has changed
	 * @param property the property that has changed
	 * @param value the value of the property
	 */
	public void preferenceChanged(String property, String value);
}

The Receiver class (that contains the XMLparser) implements IDataStorePreferenceListener.



In DataStore:

	/**
	 * Sets a property value preference on the client datastore.  If remoteAndLocal is set
	 * then the property get set on the server side as well as the client.
	 * @param property the property to set
	 * @param value the value of the property
	 * @param remoteAndLocal whether to propagate the change to the server 
	 */
	public void setPreference(String property, String value, boolean remoteAndLocal)

	/**
	 * Adds a preference change listener to the DataStore
	 * @param listener
	 */
	public void addDataStorePreferenceListener(IDataStorePreferenceListener listener){
		_dataStorePreferenceListeners.add(listener);
	}
	
	/**
	 * Removes a specific preference change listener from the Datastore
	 * @param listener
	 */
	public void removeDataStorePreferenceListener(IDataStorePreferenceListener listener){
		_dataStorePreferenceListeners.remove(listener);
	}
	
	/**
	 * Removes all the preference change listeners
	 */
	public void removeAllDataStorePreferenceListeners(){
		_dataStorePreferenceListeners.clear();
	}




Comment 8 David McKnight CLA 2008-02-28 11:17:26 EST
I've committed the changes to cvs.
Comment 9 Martin Oberhuber CLA 2008-02-28 14:07:26 EST
Created attachment 91039 [details]
Patch for reving up dstore version from 2.0.100 to 2.1.0

Note that adding API to dstore requires reving up the version number.
Since this is a compatible change, I'm reving up from 2.0.100 to 2.1.0

I also reved' up the version number in all the bundles that require dstore. For some of these it would not have been necessary, e.g. org.eclipse.rse.services.dstore would still work with 2.0 -- but since the connectorServices definitely requires 2.1 I thought it was safer to rev up all of them.
Comment 10 Martin Oberhuber CLA 2008-02-28 14:08:09 EST
Patch committed:
[220123] rev up dstore versions from 2.0.100 to 2.1.0
Comment 11 Masao Nishimoto CLA 2008-03-05 22:58:25 EST
Tested with I20080229-0710.  The server is expected to terminate after IO_SOCKET_READ_TIMEOUT + KEEP_ALIVE_RESPONSE_TIMEOUT.

When IO_SOCKET_READ_TIMEOUT > KEEP_ALIVE_RESPONSE_TIMEOUT, it takes IO_SOCKET_READ_TIMEOUT x 2 for the server to terminate.

When IO_SOCKET_READ_TIMEOUT < KEEP_ALIVE_RESPONSE_TIMEOUT, the server does not terminate.
Comment 12 David McKnight CLA 2008-03-06 11:41:33 EST
(In reply to comment #11)
> Tested with I20080229-0710.  The server is expected to terminate after
> IO_SOCKET_READ_TIMEOUT + KEEP_ALIVE_RESPONSE_TIMEOUT.
> When IO_SOCKET_READ_TIMEOUT > KEEP_ALIVE_RESPONSE_TIMEOUT, it takes
> IO_SOCKET_READ_TIMEOUT x 2 for the server to terminate.
> When IO_SOCKET_READ_TIMEOUT < KEEP_ALIVE_RESPONSE_TIMEOUT, the server does not
> terminate.

Masao, how do you go about testing these scenarios?
Comment 13 David McKnight CLA 2008-03-06 14:01:53 EST
Created attachment 91786 [details]
patch to handle case where IO_SOCKET_READ_TIMEOUT < KEEP_ALIVE_RESPONSE_TIMEOUT

Masao, with this patch does the scenario work?
Comment 14 Masao Nishimoto CLA 2008-03-07 00:12:23 EST
Now both cases take IO_SOCKET_READ_TIMEOUT x 2 for the server to terminate.

To test the scenarios, I use the debugger to stop the client at XMLgenerator.flushData().
Comment 15 Masao Nishimoto CLA 2008-03-07 02:10:30 EST
On the client, keep alive option does not work until the preference page is opened, because it is not set to the preference store on start-up.
Comment 16 David McKnight CLA 2008-03-07 09:13:35 EST
(In reply to comment #15)
> On the client, keep alive option does not work until the preference page is
> opened, because it is not set to the preference store on start-up.

I've committed code in order to initialize the preferences on startup along with the patch.  I guess the remaining issue is with why it takes IO_SOCKET_READ_TIMEOUT x 2 to terminate - or is that an issue?
Comment 17 Masao Nishimoto CLA 2008-03-10 01:28:27 EDT
The client setting now works fine.

The server terminates even if the client is active.  The first time after IO_SOCKET_READ_TIMEOUT, the keep alive thread is started.  Then the next time after another IO_SOCKET_READ_TIMEOUT, the server terminates.  There are 2 problems.

1. Even if the thread was interrupted and terminated, no new thread is created.
2. In that case, or if the thread is still active, the contents of the byte read is checked, and because nothing was read, it is treated as an error.

To solve the wrong timeout issue, how about setting the socket time out to KEEPALIVE_RESPONSE_TIMEOUT after starting the thread, and restores it to IO_SOCKET_READ_TIMEOUT after receiving something, so that the server terminates after IO_SOCKET_READ_TIMEOUT + KEEPALIVE_RESPONSE_TIMEOUT?
Comment 18 David McKnight CLA 2008-03-10 15:40:35 EDT
(In reply to comment #17)
> The client setting now works fine.
> The server terminates even if the client is active.  The first time after
> IO_SOCKET_READ_TIMEOUT, the keep alive thread is started.  Then the next time
> after another IO_SOCKET_READ_TIMEOUT, the server terminates.  There are 2
> problems.
> 1. Even if the thread was interrupted and terminated, no new thread is created.
> 2. In that case, or if the thread is still active, the contents of the byte
> read is checked, and because nothing was read, it is treated as an error.
> To solve the wrong timeout issue, how about setting the socket time out to
> KEEPALIVE_RESPONSE_TIMEOUT after starting the thread, and restores it to
> IO_SOCKET_READ_TIMEOUT after receiving something, so that the server terminates
> after IO_SOCKET_READ_TIMEOUT + KEEPALIVE_RESPONSE_TIMEOUT?

I've made some changes to cvs.  Could you try out the latest?
Comment 19 Masao Nishimoto CLA 2008-03-10 22:32:24 EDT
The fix works fine.
Comment 20 David McKnight CLA 2008-03-11 10:43:34 EDT
Marking this as fixed now.
Comment 21 Masao Nishimoto CLA 2008-03-11 22:13:48 EDT
The fix has been verified.