From 1d4d224a721e1966edaf25e0a7d619679ace5728 Mon Sep 17 00:00:00 2001 From: Florian Reimold <11774314+FlorianReimold@users.noreply.github.com> Date: Thu, 7 Nov 2024 10:37:27 +0100 Subject: [PATCH] Added is_stopped flag to server to prevent new connections and removed REUSE socket option --- fineftp-server/src/server_impl.cpp | 43 ++++++++++++++++------- fineftp-server/src/server_impl.h | 7 ++-- tests/fineftp_test/src/startstop_test.cpp | 38 ++++++++++++++++++++ 3 files changed, 72 insertions(+), 16 deletions(-) diff --git a/fineftp-server/src/server_impl.cpp b/fineftp-server/src/server_impl.cpp index 0a92a01..2a4af54 100644 --- a/fineftp-server/src/server_impl.cpp +++ b/fineftp-server/src/server_impl.cpp @@ -23,6 +23,7 @@ namespace fineftp FtpServerImpl::FtpServerImpl(const std::string& address, uint16_t port) : port_ (port) , address_ (address) + , is_stopped_ (false) , acceptor_ (io_service_) {} @@ -63,18 +64,19 @@ namespace fineftp return false; } } - - { - const std::lock_guard acceptor_lock(acceptor_mutex_); - asio::error_code ec; - acceptor_.set_option(asio::ip::tcp::acceptor::reuse_address(true), ec); - if (ec) - { - std::cerr << "Error setting reuse_address option: " << ec.message() << std::endl; - return false; - } - } + // TODO: Add the code again to use reuse_address option + //{ + // const std::lock_guard acceptor_lock(acceptor_mutex_); + + // asio::error_code ec; + // acceptor_.set_option(asio::ip::tcp::acceptor::reuse_address(true), ec); + // if (ec) + // { + // std::cerr << "Error setting reuse_address option: " << ec.message() << std::endl; + // return false; + // } + //} { const std::lock_guard acceptor_lock(acceptor_mutex_); @@ -119,6 +121,8 @@ namespace fineftp void FtpServerImpl::stop() { + is_stopped_ = true; + // Prevent new sessions from being created { const std::lock_guard acceptor_lock(acceptor_mutex_); @@ -154,6 +158,11 @@ namespace fineftp void FtpServerImpl::waitForNextFtpSession() { + if (is_stopped_) + { + return; + } + auto shutdown_callback = [weak_me = std::weak_ptr(shared_from_this())](FtpSession* session_to_delete) { if (auto me = weak_me.lock()) @@ -173,6 +182,16 @@ namespace fineftp acceptor_.async_accept(new_ftp_session->getSocket() , [weak_me = std::weak_ptr(shared_from_this()), new_ftp_session](asio::error_code ec) { + // Lock the shared pointer to this. May be a nullptr, so we need to check!!! + auto me = weak_me.lock(); + + // Before even checking the error code, check if the server has been stopped + if (!me || (me->is_stopped_)) + { + return; + } + + // Check error code if (ec) { if (ec != asio::error::operation_aborted) @@ -187,8 +206,6 @@ namespace fineftp #endif // TODO: review if this is thread safe, if right here the ftp server is shut down and the acceptor is closed. I think, that then the session will still be added to the list of open sessions and kept open. - auto me = weak_me.lock(); - if (me) { const std::lock_guard session_list_lock(me->session_list_mutex_); diff --git a/fineftp-server/src/server_impl.h b/fineftp-server/src/server_impl.h index 0011c85..3fb66f0 100644 --- a/fineftp-server/src/server_impl.h +++ b/fineftp-server/src/server_impl.h @@ -54,10 +54,11 @@ namespace fineftp void waitForNextFtpSession(); private: - UserDatabase ftp_users_; + UserDatabase ftp_users_; - const uint16_t port_; - const std::string address_; + const uint16_t port_; //! Port used on creation. May be 0. In that case, the OS will choose a port. Thus, this variable should not be used for getting the actual port. + const std::string address_; //! Address used on creation. The acceptor is bound to that address. + std::atomic is_stopped_; //! Tells whether the server has been stopped. When stopped, it will refuse to accept new connections. std::vector thread_pool_; asio::io_service io_service_; diff --git a/tests/fineftp_test/src/startstop_test.cpp b/tests/fineftp_test/src/startstop_test.cpp index 2d14599..8af4eca 100644 --- a/tests/fineftp_test/src/startstop_test.cpp +++ b/tests/fineftp_test/src/startstop_test.cpp @@ -214,6 +214,44 @@ TEST(StartStopTests, connection_count) } #endif +#if 1 +// Restart the server multiple times and check if the same socket can be reused +TEST(StartStopTests, restart_server) +{ + constexpr size_t num_restarts = 10; + + const DirPreparer dir_preparer(1, 1); + dir_preparer.create_client_files(std::filesystem::path("test.txt"), 16); + + + std::unique_ptr server; + + for (unsigned int i = 0; i < num_restarts; ++i) + { + if (server) + { + server->stop(); + server.reset(); + } + + server = std::make_unique(2121); + server->addUserAnonymous(dir_preparer.server_local_root_dir(0).string(), fineftp::Permission::All); + server->start(4); + + // use CURL to upload the file to the server + const std::string curl_command = "curl -S -s -T " + dir_preparer.client_local_root_dir(0, 0).string() + "/test.txt ftp://localhost:2121/test" + std::to_string(i) + ".txt --user anonymous:anonymous"; + const int curl_return_code = system_execute(curl_command); + + // Check return code + EXPECT_EQ(curl_return_code, 0); + + // Check existence of file + EXPECT_TRUE(std::filesystem::exists(dir_preparer.server_local_root_dir(0) / ("test" + std::to_string(i) + ".txt"))); + } +} + +#endif + #if 1 // Create a large amount of servers, upload files to them from threads and stop the servers while the upload may still be in progress TEST(StartStopTests, multiple_servers_upload_stop)