Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coprocessor: parsing error is never returned #4293

Closed
BusyJay opened this issue Feb 27, 2019 · 4 comments
Closed

coprocessor: parsing error is never returned #4293

BusyJay opened this issue Feb 27, 2019 · 4 comments
Assignees
Labels
sig/coprocessor SIG: Coprocessor type/bug The issue is confirmed as a bug.

Comments

@BusyJay
Copy link
Member

BusyJay commented Feb 27, 2019

When it fails to parse a request, the error is wrapped in an dummy handler which is returned from the function with a fake context and supposed to be executed inside read pool.

match self.try_parse_request(req, peer, is_streaming) {
Ok(v) => v,
Err(err) => {
// If there are errors when parsing requests, create a dummy request handler.
let builder =
box |_, _: &_| Ok(cop_util::ErrorRequestHandler::new(err).into_boxed());
let req_ctx = ReqContext::new(
"invalid",
kvrpcpb::Context::new(),
&[],
Duration::from_secs(60), // Large enough to avoid becoming outdated error
None,
None,
None,
);
(builder, req_ctx)

However endpoint will continue to get a snapshot first using the fake context, which will always fail as it's an invalid context.

future::result(tracker.req_ctx.deadline.check_if_exceeded())
.and_then(move |_| {
Self::async_snapshot(engine, &tracker.req_ctx.context)
.map(|snapshot| (tracker, snapshot))
})
.and_then(move |(tracker, snapshot)| {
// When snapshot is retrieved, deadline may exceed.
future::result(tracker.req_ctx.deadline.check_if_exceeded())
.map(|_| (tracker, snapshot))
})
.and_then(move |(tracker, snapshot)| {
future::result(handler_builder.call_box((snapshot, &tracker.req_ctx)))
.map(|handler| (tracker, handler))
});

So a strange error from raftstore is returned instead of the actual error which is the parsing error in the first place.

The bug is introduced in #3515, and affects v2.1.x and v3.0.x, needs to be fixed soon.

@disksing @breeswish PTAL

@BusyJay BusyJay added type/bug The issue is confirmed as a bug. sig/coprocessor SIG: Coprocessor labels Feb 27, 2019
@siddontang
Copy link
Contributor

Finally, we find this problem 😭

Btw, why parsing failed? the request is too big? @BusyJay

@disksing
Copy link
Contributor

The request can be unmarshalled in Go. I guess the problem is the DAG request recurses too deep.

@breezewish
Copy link
Member

Interesting finding. Looks like we must find a way that parsing failed errors are returned immediately instead of partially finished the pipe line.

@breezewish
Copy link
Member

Fixed in #4303

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/coprocessor SIG: Coprocessor type/bug The issue is confirmed as a bug.
Projects
None yet
Development

No branches or pull requests

4 participants