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

Fix destroy error in test_ProtoServer. #1209

Merged
merged 2 commits into from
Jan 24, 2017

Conversation

reyoung
Copy link
Collaborator

@reyoung reyoung commented Jan 22, 2017

No description provided.

@@ -163,17 +162,15 @@ int main(int argc, char** argv) {
paddle::initMain(argc, argv);
testing::InitGoogleTest(&argc, argv);

MyServer* server;
std::unique_ptr<MyServer> server;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个server变量之前没有正确的做释放。

虽然,之前终归程序会退出,在程序退出的时候,server这段内存也会还给操作系统。但是,之前这么搞的话,对象析构的顺序就是不确定的了。在GLOG开启多线程的情况下,就会导致偶发的,glog本身的全局对象已经析构了,再调用server的析构函数。而server的析构里面使用了glog来print一些信息。

这就导致程序异常退出。

Copy link
Collaborator

@wangkuiyi wangkuiyi Jan 23, 2017

Choose a reason for hiding this comment

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

根据这个描述,我理解这个PR的修改意图是:让 server 在main函数结束的时候就析构。

如果是这样,那么最朴素的C++语法既可以实现——把server定义为一个stack variable,而不是new在heap里。这样就不需要引入一个unique_ptr这样的smart pointer,也就不需要我们的读者懂得unique_ptr。

我看了一下MyServer的源码,发现最直接的解决方法貌似可以是:

  MyServer server(FLAGS_port, FLAGS_rdma_tcp == "rdma" ? 0 : -1);
  server.start();

相对这个PR里的修改:

  MyServer* server;
  std::unique_ptr<MyServer> server;
  if (FLAGS_rdma_tcp == "rdma") {
    server = new MyServer(FLAGS_port, 0);
    server.reset(new MyServer(FLAGS_port, 0));
  } else {
    server = new MyServer(FLAGS_port);
    server.reset(new MyServer(FLAGS_port));
  }
  server->start();

我这个解法貌似简单一些?

Copy link
Contributor

Choose a reason for hiding this comment

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

MyServer* server;
std::unique_ptr server;

此处有笔误? 两个相同的variable declaration?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@backyes 细致

Copy link
Collaborator

@wangkuiyi wangkuiyi left a comment

Choose a reason for hiding this comment

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

感觉这里引入一个smart pointer unqiue_ptr 貌似不是最简洁直接的解决方法。

@@ -163,17 +162,15 @@ int main(int argc, char** argv) {
paddle::initMain(argc, argv);
testing::InitGoogleTest(&argc, argv);

MyServer* server;
std::unique_ptr<MyServer> server;
Copy link
Collaborator

@wangkuiyi wangkuiyi Jan 23, 2017

Choose a reason for hiding this comment

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

根据这个描述,我理解这个PR的修改意图是:让 server 在main函数结束的时候就析构。

如果是这样,那么最朴素的C++语法既可以实现——把server定义为一个stack variable,而不是new在heap里。这样就不需要引入一个unique_ptr这样的smart pointer,也就不需要我们的读者懂得unique_ptr。

我看了一下MyServer的源码,发现最直接的解决方法貌似可以是:

  MyServer server(FLAGS_port, FLAGS_rdma_tcp == "rdma" ? 0 : -1);
  server.start();

相对这个PR里的修改:

  MyServer* server;
  std::unique_ptr<MyServer> server;
  if (FLAGS_rdma_tcp == "rdma") {
    server = new MyServer(FLAGS_port, 0);
    server.reset(new MyServer(FLAGS_port, 0));
  } else {
    server = new MyServer(FLAGS_port);
    server.reset(new MyServer(FLAGS_port));
  }
  server->start();

我这个解法貌似简单一些?

@reyoung
Copy link
Collaborator Author

reyoung commented Jan 23, 2017

最朴素的C++语法既可以实现

有道理,直接用栈变量就好了。。

@reyoung reyoung merged commit b1f09f2 into PaddlePaddle:develop Jan 24, 2017
@reyoung reyoung deleted the feature/fix_destroy_error branch January 24, 2017 07:08
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.

4 participants