Mercurial > hg > piper-cpp
changeset 158:0876b5e67afe
Improve error handling and extend tests for it
| author | Chris Cannam <cannam@all-day-breakfast.com> |
|---|---|
| date | Fri, 20 Jan 2017 22:24:44 +0000 |
| parents | 5699fca64251 |
| children | 2ada256e47e1 |
| files | vamp-capnp/VampnProto.h vamp-json/VampJson.h vamp-server/convert.cpp vamp-server/simple-server.cpp vamp-server/test.sh |
| diffstat | 5 files changed, 110 insertions(+), 57 deletions(-) [+] |
line wrap: on
line diff
--- 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
--- 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;
--- 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;
--- 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); } }
--- 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