Commit bd096087 authored by Dave Watson's avatar Dave Watson Committed by Viswanath Sivakumar

shared ptr vector sockets

Summary: promote the sockets vector to a shared_ptr, since both ServerWorkerPool and ServerBootstrap use it.  Otherwise there are destruction order issues between ServerBootstrap and any IOThreadPoolExecutor you use

Test Plan: Saw use after free in D1942242, gone after this.

Reviewed By: yfeldblum@fb.com

Subscribers: chalfant, doug, fugalh, folly-diffs@, jsedgwick, yfeldblum

FB internal diff: D1947553

Signature: t1:1947553:1427484417:5b78f5c9c70d244d3f52a6f71b6d1fab7b29d106
parent 9c19d342
...@@ -140,7 +140,7 @@ class ServerWorkerPool : public folly::wangle::ThreadPoolExecutor::Observer { ...@@ -140,7 +140,7 @@ class ServerWorkerPool : public folly::wangle::ThreadPoolExecutor::Observer {
explicit ServerWorkerPool( explicit ServerWorkerPool(
std::shared_ptr<AcceptorFactory> acceptorFactory, std::shared_ptr<AcceptorFactory> acceptorFactory,
folly::wangle::IOThreadPoolExecutor* exec, folly::wangle::IOThreadPoolExecutor* exec,
std::vector<std::shared_ptr<folly::AsyncSocketBase>>* sockets, std::shared_ptr<std::vector<std::shared_ptr<folly::AsyncSocketBase>>> sockets,
std::shared_ptr<ServerSocketFactory> socketFactory) std::shared_ptr<ServerSocketFactory> socketFactory)
: acceptorFactory_(acceptorFactory) : acceptorFactory_(acceptorFactory)
, exec_(exec) , exec_(exec)
...@@ -170,7 +170,7 @@ class ServerWorkerPool : public folly::wangle::ThreadPoolExecutor::Observer { ...@@ -170,7 +170,7 @@ class ServerWorkerPool : public folly::wangle::ThreadPoolExecutor::Observer {
std::shared_ptr<Acceptor>> workers_; std::shared_ptr<Acceptor>> workers_;
std::shared_ptr<AcceptorFactory> acceptorFactory_; std::shared_ptr<AcceptorFactory> acceptorFactory_;
folly::wangle::IOThreadPoolExecutor* exec_{nullptr}; folly::wangle::IOThreadPoolExecutor* exec_{nullptr};
std::vector<std::shared_ptr<folly::AsyncSocketBase>>* sockets_; std::shared_ptr<std::vector<std::shared_ptr<folly::AsyncSocketBase>>> sockets_;
std::shared_ptr<ServerSocketFactory> socketFactory_; std::shared_ptr<ServerSocketFactory> socketFactory_;
}; };
......
...@@ -39,7 +39,7 @@ void ServerWorkerPool::threadStopped( ...@@ -39,7 +39,7 @@ void ServerWorkerPool::threadStopped(
auto worker = workers_.find(h); auto worker = workers_.find(h);
CHECK(worker != workers_.end()); CHECK(worker != workers_.end());
for (auto& socket : *sockets_) { for (auto socket : *sockets_) {
socket->getEventBase()->runImmediatelyOrRunInEventBaseThreadAndWait( socket->getEventBase()->runImmediatelyOrRunInEventBaseThreadAndWait(
[&]() { [&]() {
socketFactory_->removeAcceptCB( socketFactory_->removeAcceptCB(
......
...@@ -132,13 +132,13 @@ class ServerBootstrap { ...@@ -132,13 +132,13 @@ class ServerBootstrap {
if (acceptorFactory_) { if (acceptorFactory_) {
workerFactory_ = std::make_shared<ServerWorkerPool>( workerFactory_ = std::make_shared<ServerWorkerPool>(
acceptorFactory_, io_group.get(), &sockets_, socketFactory_); acceptorFactory_, io_group.get(), sockets_, socketFactory_);
} else { } else {
workerFactory_ = std::make_shared<ServerWorkerPool>( workerFactory_ = std::make_shared<ServerWorkerPool>(
std::make_shared<ServerAcceptorFactory<Pipeline>>( std::make_shared<ServerAcceptorFactory<Pipeline>>(
childPipelineFactory_, childPipelineFactory_,
pipeline_), pipeline_),
io_group.get(), &sockets_, socketFactory_); io_group.get(), sockets_, socketFactory_);
} }
io_group->addObserver(workerFactory_); io_group->addObserver(workerFactory_);
...@@ -183,7 +183,7 @@ class ServerBootstrap { ...@@ -183,7 +183,7 @@ class ServerBootstrap {
}); });
}); });
sockets_.push_back(socket); sockets_->push_back(socket);
} }
void bind(folly::SocketAddress& address) { void bind(folly::SocketAddress& address) {
...@@ -268,7 +268,7 @@ class ServerBootstrap { ...@@ -268,7 +268,7 @@ class ServerBootstrap {
}); });
}); });
sockets_.push_back(socket); sockets_->push_back(socket);
} }
} }
...@@ -276,13 +276,16 @@ class ServerBootstrap { ...@@ -276,13 +276,16 @@ class ServerBootstrap {
* Stop listening on all sockets. * Stop listening on all sockets.
*/ */
void stop() { void stop() {
for (auto socket : sockets_) { // sockets_ may be null if ServerBootstrap has been std::move'd
if (sockets_) {
for (auto socket : *sockets_) {
socket->getEventBase()->runImmediatelyOrRunInEventBaseThreadAndWait( socket->getEventBase()->runImmediatelyOrRunInEventBaseThreadAndWait(
[&]() mutable { [&]() mutable {
socketFactory_->stopSocket(socket); socketFactory_->stopSocket(socket);
}); });
} }
sockets_.clear(); sockets_->clear();
}
} }
void join() { void join() {
...@@ -299,7 +302,7 @@ class ServerBootstrap { ...@@ -299,7 +302,7 @@ class ServerBootstrap {
*/ */
const std::vector<std::shared_ptr<folly::AsyncSocketBase>>& const std::vector<std::shared_ptr<folly::AsyncSocketBase>>&
getSockets() const { getSockets() const {
return sockets_; return *sockets_;
} }
std::shared_ptr<wangle::IOThreadPoolExecutor> getIOGroup() const { std::shared_ptr<wangle::IOThreadPoolExecutor> getIOGroup() const {
...@@ -318,7 +321,8 @@ class ServerBootstrap { ...@@ -318,7 +321,8 @@ class ServerBootstrap {
std::shared_ptr<wangle::IOThreadPoolExecutor> io_group_; std::shared_ptr<wangle::IOThreadPoolExecutor> io_group_;
std::shared_ptr<ServerWorkerPool> workerFactory_; std::shared_ptr<ServerWorkerPool> workerFactory_;
std::vector<std::shared_ptr<folly::AsyncSocketBase>> sockets_; std::shared_ptr<std::vector<std::shared_ptr<folly::AsyncSocketBase>>> sockets_{
std::make_shared<std::vector<std::shared_ptr<folly::AsyncSocketBase>>>()};
std::shared_ptr<AcceptorFactory> acceptorFactory_; std::shared_ptr<AcceptorFactory> acceptorFactory_;
std::shared_ptr<PipelineFactory<Pipeline>> childPipelineFactory_; std::shared_ptr<PipelineFactory<Pipeline>> childPipelineFactory_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment