From b0d0d4d073a852091dfe3625116da17e53d26597 Mon Sep 17 00:00:00 2001 From: Sandro Santilli Date: Wed, 19 Mar 2014 13:30:29 +0100 Subject: [PATCH] Reduce work on aborted requests Closes #129 Includes testcase --- NEWS.md | 1 + app/controllers/app.js | 23 ++++++++- app/models/formats/pg.js | 3 ++ test/acceptance/frontend_abort.js | 86 +++++++++++++++++++++++++++++++ test/support/assert.js | 2 +- 5 files changed, 113 insertions(+), 2 deletions(-) create mode 100644 test/acceptance/frontend_abort.js diff --git a/NEWS.md b/NEWS.md index 434351b5..f8162d1a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -16,6 +16,7 @@ Enhancements: * Upgrade node-zipfile to ~0.5.0 * Add support for node-0.10 (#132) * Fix lack of response on backend crash (#135) + * Reduce work on aborted requests (#129) 1.8.3 - 2014-02-10 ------------------ diff --git a/app/controllers/app.js b/app/controllers/app.js index 57e2694e..f384693e 100755 --- a/app/controllers/app.js +++ b/app/controllers/app.js @@ -160,6 +160,22 @@ function handleQuery(req, res) { var tableCacheItem; var requestProtocol = req.protocol; + req.aborted = false; + req.on("close", function() { + console.log("Request closed unexpectedly (aborted?)"); + req.aborted = true; // TODO: there must be a builtin way to check this + }); + + function checkAborted(step) { + if ( req.aborted ) { + var err = new Error("Request aborted during " + step); + // We'll use status 499, same as ngnix in these cases + // see http://en.wikipedia.org/wiki/List_of_HTTP_status_codes#4xx_Client_Error + err.http_status = 499; + throw err; + } + } + try { // sanitize and apply defaults to input @@ -215,6 +231,7 @@ function handleQuery(req, res) { // 5. Send formatted results back Step( function getDatabaseName() { + checkAborted('getDatabaseName'); if (_.isNull(database)) { Meta.getUserDBName(cdbuser, this); } else { @@ -268,6 +285,7 @@ function handleQuery(req, res) { }, function queryExplain(err, data){ if (err) throw err; + checkAborted('queryExplain'); if ( authenticated ) { if ( global.settings.hasOwnProperty('db_user_pass') ) { @@ -292,6 +310,7 @@ function handleQuery(req, res) { }, function setHeaders(err, result){ if (err) throw err; + checkAborted('setHeaders'); // store explain result in local Cache if ( ! tableCacheItem ) { @@ -367,6 +386,7 @@ function handleQuery(req, res) { }, function generateFormat(err, result){ if (err) throw err; + checkAborted('generateFormat'); // TODO: drop this, fix UI! sql = PSQL.window_sql(sql,limit,offset); @@ -378,7 +398,8 @@ function handleQuery(req, res) { dp: dp, skipfields: skipfields, sql: sql, - filename: filename + filename: filename, + abortChecker: checkAborted } formatter.sendResponse(opts, this); diff --git a/app/models/formats/pg.js b/app/models/formats/pg.js index cfd9aa81..5d0c5d81 100644 --- a/app/models/formats/pg.js +++ b/app/models/formats/pg.js @@ -65,6 +65,9 @@ pg.prototype.handleQueryEnd = function(result) { Step ( function packageResult() { + if ( that.opts.abortChecker ) { + that.opts.abortChecker('packageResult'); + } that.transform(result, that.opts, this); }, function sendResults(err, out){ diff --git a/test/acceptance/frontend_abort.js b/test/acceptance/frontend_abort.js new file mode 100644 index 00000000..740eec92 --- /dev/null +++ b/test/acceptance/frontend_abort.js @@ -0,0 +1,86 @@ +require('../helper'); +require('../support/assert'); + +var assert = require('assert') + , App = require(global.settings.app_root + '/app/controllers/app') + , querystring = require('querystring') + , _ = require('underscore') + , Step = require('step') + , net = require('net') + , http = require('http') + ; + +var sql_server_data_handler; +var sql_server_port = 5556; +var sql_server = net.createServer(function(c) { + c.on('data', function(d) { + console.log("SQL Server got data: " + d); + if ( sql_server_data_handler ) { + console.log("Sending data to sql_server_data_handler"); + sql_server_data_handler(null, d); + } + c.destroy(); + }); +}); + +suite('frontend abort', function() { + +suiteSetup(function(done){ + sql_server.listen(sql_server_port, done); +}); + +// See https://github.com/CartoDB/CartoDB-SQL-API/issues/129 +test('aborts request', function(done){ +//console.log("settings:"); console.dir(global.settings); + var db_host_backup = global.settings.db_host; + var db_port_backup = global.settings.db_port; + global.settings.db_host = 'localhost'; + global.settings.db_port = sql_server_port; + var app = App(); + var timeout; + Step( + function sendQuery() { + var next = this; + assert.response(app, { + url: '/api/v1/sql?q=SELECT+1', + method: 'GET', + timeout: 1, + headers: {host: 'vizzuality.localhost' } + },{}, function(res, err) { + next(err, res); + }); + }, + function checkResponse(err, res) { + assert(err); // expect timeout + assert.ok((''+err).match(/socket/), err); + sql_server_data_handler = this; + var next = this; + // If a call does not arrive to the sql server within + // the given timeout we're confident it means the request + // was successfully aborted + timeout = setTimeout(function() { next(null); }, 500); + }, + function checkSqlServerData(err, data) { + clearTimeout(timeout); + assert.ok(!data, "SQL Server was contacted no matter client abort"); + // TODO: intercept logs ? + return null; + }, + function finish(err) { + global.settings.db_host = db_host_backup; + global.settings.db_port = db_port_backup; + done(err); + } + ); +}); + +suiteTeardown(function(done) { + try { + sql_server.close(done); + } catch (er) { + console.log(er); + done(); // error expected as server is probably closed already + } +}); + +}); diff --git a/test/support/assert.js b/test/support/assert.js index ae3e6ec5..cc31c07a 100644 --- a/test/support/assert.js +++ b/test/support/assert.js @@ -133,7 +133,7 @@ assert.response = function(server, req, res, msg){ timer = setTimeout(function(){ check(); delete req.timeout; - assert.fail(msg + 'Request timed out after ' + requestTimeout + 'ms.'); + request.destroy(); // will trigger 'error' event }, requestTimeout); }