Prevent session timeout, and give better error messages (RFC 66)

[CXYS]

Customer reports that if leave the browser open for some while without any operation, the session will timeout and can’t get informed. If server is down the error message is not easy to read and user can’t figure out what happens.
The use cases would be:

1. If the Http session is timeout, the user should be informed about the fact instead of getting some script errors or meaningless error messages.
   
2. If the MapGuide Server session is timeout, the user should be informed about the fact instead of getting some script errors or some meaningless error messages.
   
3. If the connection to MapGuide Server is broken, the user should be informed by a friendly message which helps the user know what's happening instead of getting a kind of "exception stack trace" message box.

The possible solutions could be:

1. To avoid timeout (either from browser to web tier, or from web tier to MapGuide server), implement a script which pings the servers periodically to keep the both sessions alive.
   
2. Refine the code handling the exceptions like "connection broken" to provide more friendly messages.
   

For the detail implementation please see RFC 66.

[CXYS]

This patch adds a server API GetSessionTimeout()

[CXYS]

This patch adds MapAgent test for GetSessionTimeout().

[CXYS]

This patch adds supported version 2.2.0 to HttpRequestResponseHandler

[tomfukushima]

r4003 The patches were already reviewed by Trevor who had found that overall it was good; there were some minor changes required due to some unfamiliarity with the code. This review was done internally at Autodesk using code collaborator.

On scanning the code while checking it in, it looks nicely done. Nice job Christine!

[tomfukushima]

Missed some files: see r4004, r4005 for submissions of the remaining files, and changes to make sure Linux builds.

[christinebao]

Attach UpgradeWeblayoutSchema.patch for upgrade WebLayout.xsd from 1.0.0 to 1.1.0 and add a new element <EnablePingServer>.
This element will be set in MapGuide Studio to turn pinging server on/off for Ajax Viewer.

[tomfukushima]

UpgradeWeblayoutSchema.patch looks good. I have applied it: r4011.

[christinebao]

Attach FixGetSessionTimeoutAPI.patch which fixed a silly mistake. Although the code builds and works, I find that no matter what value I set for server timeout, MapAgent always gets 1200. This is because this code runs at Web Tier! MgConfiguration.GetInstance() gets webconfig.ini actually, not serverconfig.ini as expected. Thus it can't find SiteServicePropertiesSection, and returns the default value, which is 1200 always. The right way is to send command to server and get the value from serverconfig.ini.

[christinebao]

AjaxViewerPingServer.patch makes Ajax viewer pinging server to keep alive.
Once Ajax viewer loaded, it will get server time out time n seconds, and ping server at a time interval n/5 seconds. If the pinging fails after 6 times, it will stop.

[tomfukushima]

Patches applied r4036.

[ksgeograf]

I have a few comments (copied from mailing list).

1) The function name in line 455 is slightly misspelled, and referenced from line 435. 2) The webAgent and clientAgent variables are now replicated in both ajaxmappane and mainframe.templ 3) Same goes for the msie check in line 460 4) Same goes for encodeComponent function in line 406 5) Is the ParseLocalizedFloat function required? Does the server return a localized timeout value? And is it not supposed to be an int? 6) In line 434 you divide the time by 5 seconds, resulting in the number of checks pr. 5 seconds. You then pass this to setInterval. Should this not be: intervalID = window.setInterval(GetSeverSessionTimeout, num / 2.5) ? Or perhaps divided by 5 for faster checks?

I'm not sure 2,3,4 is an issue, or it makes sense to move it around, but eventually someone will fix something in one place and leave the other one broken.

[christinebao]

Attach patch AjaxViewerPingServer_Fix.patch for fixing problem mentioned by Kenneth's comments:

1. Update misspelled function name from GetSeverSessionTimeout() to GetServerSessionTimeout().
   
2. Rename webAgent and clientAgent to avoid conflicting with ajaxmappane.templ. (Sorry, I didn't know the two templ files can't have same variable name)
   
3. msie is not used.
   
4. Rename function name encodeComponent() to encodeComponentName().
   
5. ParseLocalizedFloat() is a function used in bufferui.templ to parse float, and I copied it here. Also, the name is changed to ParseLocalizedFloatNum().
   
6. I'm not sure about your meaning. The default server timeout is 1200 seconds, and 1200 seconds / 5 = 240 seconds = 4 minutes, that's browser pings server every 4 minutes. setInterval requires millisecond as unit, that's why *1000.

In this patch there are also fixes for:

1. Append time in request URL. This is because IE will cache the request and not send to server. By appending current time the request URL is always different and no cache happens.
   
2. IE should also use XMLHttpRequest instead of ActiveXObject("Microsoft.XMLHTTP"), otherwise handler can't work properly.

Thank you for Kenneth's review again!

[chrisclaydon]

Submitted Christine's latest patch:

​http://trac.osgeo.org/mapguide/changeset/4068

[christinebao]

MapGuide customers report a severe defect that Flexible Web Layout reports unreadable error message.

For example, if the server is down while user do operation, (s)he will get a dialog with long error messages: "FATAL: xml2json: invalid XML document: MgConnectionFailedException? : ​http://127.0.0.1/mapguide2010/mapagent/mapagent.fcgi?version=1.0.0&locale=en&clientagent=Fusion%20Viewer&operation=QUERYMAPFEATURES&session=40d3e074-3ee5-102c-8000-005056c00008_en_7F0000010AFC0AFB0AFA&mapname=Sheboygan4a3b609791c82&geometry=POLYGON((-87.730254250931%2043.73763292302%2C%20-87.730254250931%2043.737069942268%2C%20-87.729691270179%2043.737069942268%2C%20-87.729691270179%2043.73763292302%2C%20-87.730254250931%2043.73763292302))&maxFeatures=1&persist=0&selectionVariant=INTERSECTS&layerNames=&layerAttributeFilter=5 type=0".

This error message is helpless for end users. Customers report that they want a more user friendly message.

This message is from Fusion error handling code (fusion.js line 647):

if (r.status >= 400) {

Fusion.reportError(new Fusion.Error(Fusion.Error.FATAL,

'xml2json: invalid XML document: ' + r.statusText + " : " + r.request.url));

return;

}

r.statusText is exception type from MapGuide, for example MgConnectionFailedException.
r.request.url is the long URL.

To improve this:

1. MapGuide should set the exception message instead of exception type in r.statusText. For example, r.statusText should be "Cannot establish connection" instead of "MgConnectionFailedException". These messages are localized and readable to end users.
   
2. MapGuide templates handle the error, parse it to readable message and show to user.

By doing so end user can get a readable error message. For example "FATAL: Cannot establish connection"

[ksgeograf]

In (4) and (5), I think you can use the same function/variable name, but you are replicating the code. My comment was that this is hard to maintain, and -in a perfect world-, the two templ files would share the function, so it only has be maintained in one place.

For (6), I read num / 5 * 1000 as: num / 5 seconds, which is not what is meant. I would suggest a comment like:
Ping server 5 times each period. Timeout is returned in seconds.

[christinebao]

Hi Kenneth,

I considered to share the same functions between templates, in that way we don't duplicate code. However the problem is I can't find a suitable place to put these functions. It seems there are js files included by the templates and shared, such as browserdetect.js, but none of them are suitable for encodeComponent() and ParseLocalizedFloat(). What's your suggestion?

I'll add comments for the ping server interval. Thanks for your suggestion.

Thanks & regards, Christine

Replying to ksgeograf:

In (4) and (5), I think you can use the same function/variable name, but you are replicating the code. My comment was that this is hard to maintain, and -in a perfect world-, the two templ files would share the function, so it only has be maintained in one place.

For (6), I read num / 5 * 1000 as: num / 5 seconds, which is not what is meant. I would suggest a comment like:
Ping server 5 times each period. Timeout is returned in seconds.

[ksgeograf]

I would create a "util.js" file, if there is no such file already.

[christinebao]

Hi Kenneth,

Would you please review patch ​http://trac.osgeo.org/mapguide/attachment/ticket/1026/Util.patch. It's appreciated if you can submit it if everything is ok.

Thanks & regards, Christine

Replying to ksgeograf:

I would create a "util.js" file, if there is no such file already.

[ksgeograf]

Looks good, unfortunately I don't have commit rights to the MapGuide repository, only the Maestro part, but I'm sure that Tom will submit it.

[tomfukushima]

Submitted as r4077. I noticed that there was a comment in the util.js file but left it in. I think that we don't put any extraneous things like comments in these files because it unnecessarily consumes bandwith (well, that's what it seems from looking at the other files).

If documentation becomes an issue, perhaps we should start looking into creating documented .js and template files and then postprocessing them so that that are lean for the web. Much like Fusion does in creating the compressed fusion files.

[chrisclaydon]

Submitted Christine's latest patch:

​http://trac.osgeo.org/mapguide/changeset/4092

This updated the error message handling for Fusion errors in the MapGuide templates.

[christinebao]

Reopen this ticket for refining code.

Previously mainframe.templ send request to fcgi and get server timeout by parsing the return value. This works but indirect. In this submission it’s refined to use web tier API to get the value.

Patch ​http://trac.osgeo.org/mapguide/attachment/ticket/1026/RefindPingServer.patch is attached.

[ksgeograf]

Clever improvement, the overall impact on performance should be low, but removing the need to parse the value is good.

[tomfukushima]

submitted with r4142.

[christinebao]

Add patch ​http://trac.osgeo.org/mapguide/attachment/ticket/1026/RefineErrorMessage.patch.

There are two fixes in this patch:

1. If the error message contains "\n" or "\t", the string in status text will be converted into "
   n" and "
   t". Here format the message again to show the correct style.
   
2. Remove "Fusion Error" stuff for those message come from MapGuide exception because it's too technical.

[tomfukushima]

Submitted patch as r4151.

[christinebao]

Attach ​http://trac.osgeo.org/mapguide/attachment/ticket/1026/FixOperationVersion.patch.

The operation version should be 1.0.0 for new added API, and it has nothing to do with MapGuide version. This patch fixed the API GetSessionTimeout()'s version from 2.2.0 to 1.0.0.

[tomfukushima]

Submitted: r4155