# HG changeset patch
# User Chris Cannam <cannam@all-day-breakfast.com>
# Date 1484951084 0
# Node ID 0876b5e67afe426586a22098bae3a709c74557da
# Parent  5699fca64251dc6652d021a86af062f743c78702
Improve error handling and extend tests for it

diff -r 5699fca64251 -r 0876b5e67afe vamp-capnp/VampnProto.h
--- a/vamp-capnp/VampnProto.h	Fri Jan 20 22:24:11 2017 +0000
+++ b/vamp-capnp/VampnProto.h	Fri Jan 20 22:24:44 2017 +0000
@@ -874,7 +874,13 @@
         }
 
         e.setCode(0);
-        e.setMessage(std::string("error in ") + type + " request: " + errorText);
+
+        if (responseType == RRType::NotValid) {
+            e.setMessage(errorText);
+        } else {
+            e.setMessage
+                (std::string("error in ") + type + " request: " + errorText);
+        }
     }
 
     static void
diff -r 5699fca64251 -r 0876b5e67afe vamp-json/VampJson.h
--- a/vamp-json/VampJson.h	Fri Jan 20 22:24:11 2017 +0000
+++ b/vamp-json/VampJson.h	Fri Jan 20 22:24:44 2017 +0000
@@ -1314,8 +1314,13 @@
 
         json11::Json::object eo;
         eo["code"] = 0;
-        eo["message"] = 
-            std::string("error in ") + type + " request: " + errorText;
+
+        if (responseType == RRType::NotValid) {
+            eo["message"] = errorText;
+        } else {
+            eo["message"] = 
+                std::string("error in ") + type + " request: " + errorText;
+        }
 
         jo["method"] = type;
         jo["error"] = eo;
diff -r 5699fca64251 -r 0876b5e67afe vamp-server/convert.cpp
--- a/vamp-server/convert.cpp	Fri Jan 20 22:24:11 2017 +0000
+++ b/vamp-server/convert.cpp	Fri Jan 20 22:24:44 2017 +0000
@@ -187,7 +187,7 @@
 }
 
 RequestOrResponse
-readRequestJson(string &err)
+readRequestJson(string &err, bool &eof)
 {
     RequestOrResponse rr;
     rr.direction = RequestOrResponse::Request;
@@ -195,7 +195,7 @@
     string input;
     if (!getline(cin, input)) {
         // the EOF case, not actually an error
-        rr.type = RRType::NotValid;
+        eof = true;
         return rr;
     }
     
@@ -272,7 +272,7 @@
 }
 
 RequestOrResponse
-readResponseJson(string &err)
+readResponseJson(string &err, bool &eof)
 {
     RequestOrResponse rr;
     rr.direction = RequestOrResponse::Response;
@@ -280,7 +280,7 @@
     string input;
     if (!getline(cin, input)) {
         // the EOF case, not actually an error
-        rr.type = RRType::NotValid;
+        eof = true;
         return rr;
     }
 
@@ -361,6 +361,7 @@
                 (rr.finishResponse, mapper, serialisation, id);
             break;
         case RRType::NotValid:
+            j = VampJson::fromError(rr.errorText, rr.type, id);
             break;
         }
     }
@@ -472,8 +473,6 @@
         VampnProto::readRpcResponse_Finish(rr.finishResponse, reader, mapper);
         break;
     case RRType::NotValid:
-        // error
-        rr.success = false;
         VampnProto::readRpcResponse_Error(errorCode, rr.errorText, reader);
         break;
     }
@@ -513,6 +512,7 @@
             VampnProto::buildRpcResponse_Finish(builder, rr.finishResponse, mapper);
             break;
         case RRType::NotValid:
+            VampnProto::buildRpcResponse_Error(builder, rr.errorText, rr.type);
             break;
         }
     }
@@ -521,22 +521,23 @@
 }
 
 RequestOrResponse
-readInputJson(RequestOrResponse::Direction direction, string &err)
+readInputJson(RequestOrResponse::Direction direction, string &err, bool &eof)
 {
     if (direction == RequestOrResponse::Request) {
-        return readRequestJson(err);
+        return readRequestJson(err, eof);
     } else {
-        return readResponseJson(err);
+        return readResponseJson(err, eof);
     }
 }
 
 RequestOrResponse
-readInputCapnp(RequestOrResponse::Direction direction)
+readInputCapnp(RequestOrResponse::Direction direction, bool &eof)
 {
     static kj::FdInputStream stream(0); // stdin
     static kj::BufferedInputStreamWrapper buffered(stream);
 
     if (buffered.tryGetReadBuffer() == nullptr) {
+        eof = true;
         return {};
     }
     
@@ -548,15 +549,17 @@
 }
 
 RequestOrResponse
-readInput(string format, RequestOrResponse::Direction direction)
+readInput(string format, RequestOrResponse::Direction direction, bool &eof)
 {
+    eof = false;
+    
     if (format == "json") {
         string err;
-        auto result = readInputJson(direction, err);
+        auto result = readInputJson(direction, err, eof);
         if (err != "") throw runtime_error(err);
         else return result;
     } else if (format == "capnp") {
-        return readInputCapnp(direction);
+        return readInputCapnp(direction, eof);
     } else {
         throw runtime_error("unknown input format \"" + format + "\"");
     }
@@ -649,13 +652,12 @@
 
         try {
 
-            RequestOrResponse rr = readInput(informat, direction);
-
-            // NotValid without an exception indicates EOF:
-            if (rr.type == RRType::NotValid) break;
+            bool eof = false;
+            RequestOrResponse rr = readInput(informat, direction, eof);
+            if (eof) break;
 
             writeOutput(outformat, rr);
-            
+
         } catch (std::exception &e) {
 
             cerr << "Error: " << e.what() << endl;
diff -r 5699fca64251 -r 0876b5e67afe vamp-server/simple-server.cpp
--- a/vamp-server/simple-server.cpp	Fri Jan 20 22:24:11 2017 +0000
+++ b/vamp-server/simple-server.cpp	Fri Jan 20 22:24:44 2017 +0000
@@ -245,7 +245,7 @@
 }
 
 RequestOrResponse
-readRequestJson(string &err)
+readRequestJson(string &err, bool &eof)
 {
     RequestOrResponse rr;
     rr.direction = RequestOrResponse::Request;
@@ -253,7 +253,7 @@
     string input;
     if (!getline(cin, input)) {
         // the EOF case, not actually an error
-        rr.type = RRType::NotValid;
+        eof = true;
         return rr;
     }
     
@@ -339,14 +339,15 @@
 }
 
 void
-writeExceptionJson(const exception &e, RRType type)
+writeExceptionJson(const exception &e, RRType type, RequestOrResponse::RpcId id)
 {
-    Json j = VampJson::fromError(e.what(), type, Json());
+    Json jid = writeJsonId(id);
+    Json j = VampJson::fromError(e.what(), type, jid);
     cout << j.dump() << endl;
 }
 
 RequestOrResponse
-readRequestCapnp()
+readRequestCapnp(bool &eof)
 {
     RequestOrResponse rr;
     rr.direction = RequestOrResponse::Request;
@@ -355,7 +356,7 @@
     static kj::BufferedInputStreamWrapper buffered(stream);
 
     if (buffered.tryGetReadBuffer() == nullptr) {
-        rr.type = RRType::NotValid;
+        eof = true;
         return rr;
     }
 
@@ -430,10 +431,12 @@
 }
 
 void
-writeExceptionCapnp(const exception &e, RRType type)
+writeExceptionCapnp(const exception &e, RRType type, RequestOrResponse::RpcId id)
 {
     capnp::MallocMessageBuilder message;
     piper::RpcResponse::Builder builder = message.initRoot<piper::RpcResponse>();
+
+    buildId(builder, id);
     VampnProto::buildRpcResponse_Exception(builder, e, type);
     
     writeMessageToFd(1, message);
@@ -472,6 +475,10 @@
     case RRType::Configure:
     {
         auto &creq = request.configurationRequest;
+        if (!creq.plugin) {
+            throw runtime_error("unknown plugin handle supplied to configure");
+        }
+            
         auto h = mapper.pluginToHandle(creq.plugin);
         if (mapper.isConfigured(h)) {
             throw runtime_error("plugin has already been configured");
@@ -490,6 +497,10 @@
     case RRType::Process:
     {
         auto &preq = request.processRequest;
+        if (!preq.plugin) {
+            throw runtime_error("unknown plugin handle supplied to process");
+        }
+
         auto h = mapper.pluginToHandle(preq.plugin);
         if (!mapper.isConfigured(h)) {
             throw runtime_error("plugin has not been configured");
@@ -521,6 +532,10 @@
     case RRType::Finish:
     {
         auto &freq = request.finishRequest;
+        if (!freq.plugin) {
+            throw runtime_error("unknown plugin handle supplied to finish");
+        }
+
         response.finishResponse.plugin = freq.plugin;
 
         auto h = mapper.pluginToHandle(freq.plugin);
@@ -547,13 +562,13 @@
 }
 
 RequestOrResponse
-readRequest(string format)
+readRequest(string format, bool &eof)
 {
     if (format == "capnp") {
-        return readRequestCapnp();
+        return readRequestCapnp(eof);
     } else if (format == "json") {
         string err;
-        auto result = readRequestJson(err);
+        auto result = readRequestJson(err, eof);
         if (err != "") throw runtime_error(err);
         else return result;
     } else {
@@ -576,13 +591,13 @@
 }
 
 void
-writeException(string format, const exception &e, RRType type)
+writeException(string format, const exception &e, RRType type, RequestOrResponse::RpcId id)
 {
     resumeOutput();
     if (format == "capnp") {
-        writeExceptionCapnp(e, type);
+        writeExceptionCapnp(e, type, id);
     } else if (format == "json") {
-        writeExceptionJson(e, type);
+        writeExceptionJson(e, type, id);
     } else {
         throw runtime_error("unknown output format \"" + format + "\"");
     }
@@ -644,10 +659,10 @@
         
         try {
 
-            request = readRequest(format);
+            bool eof = false;
+            request = readRequest(format, eof);
             
-            // NotValid without an exception indicates EOF:
-            if (request.type == RRType::NotValid) {
+            if (eof) {
                 if (debug) {
                     cerr << myname << " " << pid << ": eof reached, exiting" << endl;
                 }
@@ -659,7 +674,28 @@
                      << int(request.type)
                      << endl;
             }
+            
+        } catch (exception &e) {
 
+            if (debug) {
+                cerr << myname << " " << pid << ": error: " << e.what() << endl;
+            }
+
+            writeException(format, e, request.type, request.id);
+
+            if (format == "capnp") {
+                // Don't try to continue; we can't recover from a
+                // mangled input stream. However, we can return a
+                // successful error code because we are reporting the
+                // status in our Capnp output stream instead
+                if (debug) {
+                    cerr << myname << " " << pid << ": not attempting to recover from capnp parse problems, exiting" << endl;
+                }
+                exit(0);
+            }
+        }
+
+        try {
             RequestOrResponse response = handleRequest(request, debug);
             response.id = request.id;
 
@@ -689,18 +725,7 @@
                 cerr << myname << " " << pid << ": error: " << e.what() << endl;
             }
 
-            writeException(format, e, request.type);
-
-            if (format == "capnp") {
-                // Don't try to continue; we can't recover from a
-                // mangled input stream. However, we can return a
-                // successful error code because we are reporting the
-                // status in our Capnp output stream instead
-                if (debug) {
-                    cerr << myname << " " << pid << ": not attempting to recover from capnp parse problems, exiting" << endl;
-                }
-                exit(0);
-            }
+            writeException(format, e, request.type, request.id);
         }
     }
 
diff -r 5699fca64251 -r 0876b5e67afe vamp-server/test.sh
--- a/vamp-server/test.sh	Fri Jan 20 22:24:11 2017 +0000
+++ b/vamp-server/test.sh	Fri Jan 20 22:24:44 2017 +0000
@@ -2,8 +2,11 @@
 
 set -eu
 
-piperdir=../piper
-vampsdkdir=../vamp-plugin-sdk
+mydir=$(dirname "$0")
+
+piperdir="$mydir"/../../piper
+vampsdkdir="$mydir"/../../vamp-plugin-sdk
+bindir="$mydir"/../bin
 schemadir="$piperdir"/json/schema
 
 if [ ! -d "$schemadir" ]; then
@@ -51,23 +54,32 @@
     validate "$respfile" "rpcresponse"
 }
 
+# NB this list of commands includes a couple that are expected to fail
+# (process before configure, configure with nonexistent handle, finish
+# same handle twice)
 cat > "$input" <<EOF
 {"method":"list"}
 {"method":"list","params": {"from":["vamp-example-plugins","something-nonexistent"]}}
 {"method":"list","params": {"from":["something-nonexistent"]}}
 {"method":"load","id":6,"params": {"key":"vamp-example-plugins:percussiononsets","inputSampleRate":44100,"adapterFlags":["AdaptInputDomain","AdaptBufferSize"]}}
+{"method":"process","params": {"handle": 1, "processInput": { "timestamp": {"s": 0, "n": 0}, "inputBuffers": [ [1,2,3,4,5,6,7,8] ]}}}
 {"method":"configure","id":"weevil","params":{"handle":1,"configuration":{"blockSize": 8, "channelCount": 1, "parameterValues": {"sensitivity": 40, "threshold": 3}, "stepSize": 8}}}
+{"method":"configure","id":9,"params":{"handle":-9999,"configuration":{"blockSize": 8, "channelCount": 1, "parameterValues": {"sensitivity": 40, "threshold": 3}, "stepSize": 8}}}
 {"method":"process","params": {"handle": 1, "processInput": { "timestamp": {"s": 0, "n": 0}, "inputBuffers": [ [1,2,3,4,5,6,7,8] ]}}}
 {"method":"finish","params": {"handle": 1}}
+{"method":"finish","id":"blah","params": {"handle": 1}}
 EOF
 
 # Expected output, apart from the plugin list which seems a bit
 # fragile to check here
 cat > "$expected" <<EOF
 {"id": 6, "jsonrpc": "2.0", "method": "load", "result": {"defaultConfiguration": {"blockSize": 1024, "channelCount": 1, "parameterValues": {"sensitivity": 40, "threshold": 3}, "stepSize": 1024}, "handle": 1, "staticData": {"basic": {"description": "Detect percussive note onsets by identifying broadband energy rises", "identifier": "percussiononsets", "name": "Simple Percussion Onset Detector"}, "basicOutputInfo": [{"description": "Percussive note onset locations", "identifier": "onsets", "name": "Onsets"}, {"description": "Broadband energy rise detection function", "identifier": "detectionfunction", "name": "Detection Function"}], "category": ["Time", "Onsets"], "copyright": "Code copyright 2006 Queen Mary, University of London, after Dan Barry et al 2005.  Freely redistributable (BSD license)", "inputDomain": "TimeDomain", "key": "vamp-example-plugins:percussiononsets", "maker": "Vamp SDK Example Plugins", "maxChannelCount": 1, "minChannelCount": 1, "parameters": [{"basic": {"description": "Energy rise within a frequency bin necessary to count toward broadband total", "identifier": "threshold", "name": "Energy rise threshold"}, "defaultValue": 3, "extents": {"max": 20, "min": 0}, "unit": "dB", "valueNames": []}, {"basic": {"description": "Sensitivity of peak detector applied to broadband detection function", "identifier": "sensitivity", "name": "Sensitivity"}, "defaultValue": 40, "extents": {"max": 100, "min": 0}, "unit": "%", "valueNames": []}], "programs": [], "version": 2}}}
+{"error": {"code": 0, "message": "error in process request: plugin has not been configured"}, "jsonrpc": "2.0", "method": "process"}
 {"id": "weevil", "jsonrpc": "2.0", "method": "configure", "result": {"handle": 1, "outputList": [{"basic": {"description": "Percussive note onset locations", "identifier": "onsets", "name": "Onsets"}, "configured": {"binCount": 0, "binNames": [], "hasDuration": false, "sampleRate": 44100, "sampleType": "VariableSampleRate", "unit": ""}}, {"basic": {"description": "Broadband energy rise detection function", "identifier": "detectionfunction", "name": "Detection Function"}, "configured": {"binCount": 1, "binNames": [""], "hasDuration": false, "quantizeStep": 1, "sampleRate": 86.1328125, "sampleType": "FixedSampleRate", "unit": ""}}]}}
+{"error": {"code": 0, "message": "error in configure request: unknown plugin handle supplied to configure"}, "id": 9, "jsonrpc": "2.0", "method": "configure"}
 {"jsonrpc": "2.0", "method": "process", "result": {"features": {}, "handle": 1}}
 {"jsonrpc": "2.0", "method": "finish", "result": {"features": {"detectionfunction": [{"featureValues": [0], "timestamp": {"n": 11609977, "s": 0}}]}, "handle": 1}}
+{"error": {"code": 0, "message": "error in finish request: unknown plugin handle supplied to finish"}, "id": "blah", "jsonrpc": "2.0", "method": "finish"}
 EOF
 
 # We run the whole test twice, once with the server in Capnp mode
@@ -77,7 +89,7 @@
 #debugflag=-d
 debugflag=
 
-for format in json capnp ; do
+for format in json capnp ; do  # nb must be json first: see comment at end of loop
 
     ( export VAMP_PATH="$vampsdkdir"/examples ;
       while read request ; do
@@ -85,11 +97,11 @@
           echo "$request"
       done |
           if [ "$format" = "json" ]; then
-              bin/piper-vamp-simple-server $debugflag json
+              "$bindir"/piper-vamp-simple-server $debugflag json
           else
-              bin/piper-convert request -i json -o capnp |
-                  bin/piper-vamp-simple-server $debugflag capnp |
-                  bin/piper-convert response -i capnp -o json
+              "$bindir"/piper-convert request -i json -o capnp |
+                  "$bindir"/piper-vamp-simple-server $debugflag capnp |
+                  "$bindir"/piper-convert response -i capnp -o json
           fi |
           while read response ; do
               echo "$response" >> "$allrespfile"
@@ -102,7 +114,7 @@
 
     echo "Checking response contents against expected contents..."
     if ! cmp "$obtained" "$expected"; then
-        diff -u1 "$obtained" "$expected"
+        diff -U 1 "$obtained" "$expected"
     else
         echo "OK"
     fi
@@ -142,6 +154,9 @@
     
     rm "$allrespfile"
 
+    # The capnp output doesn't preserve the method name in error
+    # responses, so replace those now that we've done the json test
+    perl -i -p -e 's/(error.*"method": )"[^"]*"/$1"invalid"/' "$expected"
 done
 
 echo "Tests succeeded"  # set -e at top should ensure we don't get here otherwise