liaoxin01 commented on code in PR #68462:
URL: https://github.com/apache/doris/pull/68462#discussion_r4260951385
##########
be/src/service/http/ev_http_server.cpp:
##########
@@ -66,7 +89,7 @@ static void on_request(struct evhttp_request* ev_req, void*
arg) {
// In this case, request's on_header return -1
return;
}
- request->handler()->handle(request);
+ catch_handler_exception("handle", request, [request] {
request->handler()->handle(request); });
Review Comment:
Resolved by dropping the boundary guard instead. On reflection the catch-all
in `ev_http_server.cpp` was out of proportion to the bug and, as both of your
P1s show, it created a new problem class of its own (unanswered requests,
retained contexts, reply-state tracking across every send site). It is gone.
The four reachable unguarded parses behind the http port are now each guarded
at the call site, which is what the rest of the codebase already does, so no
handler throws in the first place: `Content-Length` and `wal_id` in
`http_stream.cpp`, `auth_code` in `parse_basic_auth()`, and `reset_value` in
`SetJeHeapProfileResetActions::handle()` - the `/jeheap/reset/not_a_number`
case you named. Note `segment_index_id` in `download_binlog_action.cpp` is
already inside a try/catch, that claim in my earlier PR description was wrong
and has been corrected.
##########
be/src/service/http/ev_http_server.cpp:
##########
@@ -49,9 +50,31 @@ struct evhttp;
namespace doris {
+// libevent runs the hooks below from its own C frames, so an exception
escaping a
+// handler would unwind through them and terminate the whole BE process.
Handlers are
+// expected to report failures through Status, and this is the last resort
that keeps a
+// missed one from taking the process down: the exception is logged and the
request is
+// failed, never the BE.
+// raw_path() is logged instead of uri() to keep query string credentials out
of the log.
+template <typename Fn>
+static bool catch_handler_exception(const char* hook, const HttpRequest*
request, Fn&& fn) {
+ try {
+ fn();
+ return true;
+ } catch (const std::exception& e) {
+ LOG(WARNING) << "http handler throws exception in " << hook
+ << ", path=" << request->raw_path() << ", error=" <<
e.what();
+ } catch (...) {
+ LOG(WARNING) << "http handler throws unknown exception in " << hook
+ << ", path=" << request->raw_path();
+ }
+ return false;
+}
+
static void on_chunked(struct evhttp_request* ev_req, void* param) {
HttpRequest* request = (HttpRequest*)ev_req->on_free_cb_arg;
- request->handler()->on_chunk_data(request);
+ catch_handler_exception("on_chunk_data", request,
Review Comment:
Resolved the same way: the chunk callback boundary guard is removed, so
there is no swallowed exception to poison the request around. `on_chunk_data`
in `http_stream.cpp` now parses `wal_id` with `safe_stoll()` and records the
failure in `ctx->status`, which the existing code in that callback already does
for its other failures, and the terminal handler honours it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
