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

bugfix: fix coredump issue when future_execute() return network error #118

Merged
merged 1 commit into from Aug 15, 2023

Conversation

rayhuang90
Copy link
Contributor

bugfix: 修复future_execute()错误时,ExecutionResponse.spaceName == nullptr,直接解引用导致的coredump问题
bugfix: fix coredump issue when future_execute() return network error. In such case, ExecutionResponse.spaceName == nullptr。There is segment fault at SessionPool.cpp@line45 and line65。

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

segement fault:
image
full log:
coredump.log
source code:

ExecutionResponse SessionPool::execute(const std::string &stmt) {
  auto result = getIdleSession();
  if (result.second) {
    auto resp = result.first.execute(stmt);
    if (*resp.spaceName != config_.spaceName_) {    // line45
      // switch to origin space
      result.first.execute("USE " + config_.spaceName_);
    }
    giveBack(std::move(result.first));
    return resp;
  }
...
}

根因 / root cause
Connection.cpp @ line145

ExecutionResponse Connection::execute(int64_t sessionId, const std::string &stmt) {
  ExecutionResponse resp;
  try {
    resp = client_->future_execute(sessionId, stmt).get();
  } catch (const std::exception &ex) {
    resp = ExecutionResponse{
        ErrorCode::E_RPC_FAILURE, 0, nullptr, nullptr, std::make_unique<std::string>(ex.what())};
  }
  return resp;
}

How do you solve it?

增加 spaceName 非空判断,不无脑解引用
check ExecutionResponse.spaceName is nullptr before compared with config_.spaceName_

Special notes for your reviewer, ex. impact of this fix, design document, etc:

@CLAassistant
Copy link

CLAassistant commented Jul 18, 2023

CLA assistant check
All committers have signed the CLA.

@rayhuang90
Copy link
Contributor Author

@yixinglu
这些代码检查流水线失败了,帮忙看一下?我预期我修改的代码,毕竟才加了2个判断语句,不会触发这些流水线告警才对。
hi, please check the code merge request pipeline error. There are only two line of code modification. No warning or pipeline error is expected. Please let me know if i can be of any help.

@yixinglu yixinglu requested a review from Shylock-Hg July 20, 2023 03:24
@yixinglu
Copy link

@yixinglu 这些代码检查流水线失败了,帮忙看一下?我预期我修改的代码,毕竟才加了2个判断语句,不会触发这些流水线告警才对。 hi, please check the code merge request pipeline error. There are only two line of code modification. No warning or pipeline error is expected. Please let me know if i can be of any help.

don't worry about these failures which will be fixed later, thanks for your contribution! cc @Shylock-Hg

@@ -62,7 +62,7 @@ ExecutionResponse SessionPool::executeWithParameter(
auto result = getIdleSession();
if (result.second) {
auto resp = result.first.executeWithParameter(stmt, parameters);
if (*resp.spaceName != config_.spaceName_) {
if (resp.spaceName != nullptr && *resp.spaceName != config_.spaceName_) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resp.spaceName == nullptr || *resp.spaceName != config_.spaceName_

@songqing
Copy link
Contributor

@Shylock-Hg @rayhuang90 We have met the same problem, could you modify and merge this PR?

@Shylock-Hg Shylock-Hg merged commit 11d1649 into vesoft-inc:master Aug 15, 2023
7 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants