eclipse-ecal / fineftp-server

📦 C++ FTP Server Library for Windows 🪟, Linux 🐧 & more 💾
MIT License
311 stars 64 forks source link

Stop server does not free the accept socket port (on Windows) #70

Open monsieurgustav opened 2 months ago

monsieurgustav commented 2 months ago

Hi, I have an unsual use-case where I must stop and restart the FTP server during the app lifetime. But when I stop it the 1st time, I can observe that the listening port is still busy. (using "netstat -abn -p tcp") The next start works without an error, but no connection is actually accepted. It happens even if no client connect to the server. Do you have a clue? Best regards, Guillaume

FlorianReimold commented 1 month ago

Thanks for the report! I will take a look and try to reproduce it. I just don't have the time at the moment, so it will take be a little while. Thanks for the patience!

FlorianReimold commented 3 weeks ago

Hi @monsieurgustav, when you write you "stop" the sever, what exactly are you doing? Killing an application? Deleting the C++ object? Is it possible for you to post some code that triggers the issue?

monsieurgustav commented 3 weeks ago

Sure, here it is:

void restartFtpServer(std::unique_ptr<fineftp::FtpServer>& ftpServer, const QString& folder, uint16_t port, size_t threadCount = 1)
    {
        if (ftpServer)
        {
            ftpServer->stop();
            ftpServer.reset();
        }

        ftpServer = std::make_unique<fineftp::FtpServer>(port);
        ftpServer->addUserAnonymous(folder.toUtf8().toStdString(), fineftp::Permission::ReadOnly);
        ftpServer->start(threadCount);
    }

I call this function when the root folder of the FTP server changes. (I don't think there is another way to do it)

If I remember correctly, one may need to connect (anonymously) to the server before it is destroyed/restarted to trigger the issue. I use WinSCP to connect.

FlorianReimold commented 2 weeks ago

I confirmed the issue with wireshark. Indeed, the socket is not closed properly. I think the reason is the following code:

https://github.com/eclipse-ecal/fineftp-server/blob/65884209c5af28504069b53ee7bb164417fbb9dd/fineftp-server/src/server_impl.cpp#L114-L122

The server stops the io_service, which makes the application to rely on the OS to clean up the sockets. And that may not happen fast enough.

I will check how to fix that bug. For future references: You can also check the existence of that bug in Wireshark, as it will show the improperly terminated connection in red: image

My test application looks like follows (Windows only, due to hacky system commands):

#include <chrono>
#include <fineftp/server.h>

#include <string>
#include <thread>

void restartFtpServer(std::unique_ptr<fineftp::FtpServer>& ftpServer, const std::string& folder, uint16_t port, size_t threadCount = 1)
{
  if (ftpServer)
  {
    ftpServer->stop();
    ftpServer.reset();
  }

  ftpServer = std::make_unique<fineftp::FtpServer>(port);
  ftpServer->addUserAnonymous(folder, fineftp::Permission::ReadOnly);
  ftpServer->start(threadCount);
}

int main() {
  std::unique_ptr<fineftp::FtpServer> ftpServer;

  restartFtpServer(ftpServer, "C:\\", 2121, 1);

  system("pause");

  restartFtpServer(ftpServer, "D:\\", 2121, 1);

  system("pause");
}
monsieurgustav commented 2 weeks ago

Did you try to pause between the stop/start and wait a long time? (30s? 1mn?) Does the socket/port frees itself at some point?

FlorianReimold commented 2 weeks ago

Actually, I did not experience any issue. In my test, fineftp was able to re-open the port immediately (code above). I didn't check netstat though, as I already confirmed the bug with Wireshark.

What needs to be done is entirely drop the io_service_.stop();. Without the io_service, sockets will not be able to properly shut down. Stopping the FTP Server must do the following instead:

  1. Prevent new sessions from being accepted
  2. Stop all existing FTP sessions
  3. Join all threads without stopping the io_service. This will wait for the io_service to run out of work on its own.
FlorianReimold commented 1 day ago

@monsieurgustav: I fixed the socket-close issue in a branch: https://github.com/eclipse-ecal/fineftp-server/tree/hotfix/properly_close_sockets

It's not perfect, yet, but ready for a test. Can you test it out?

Btw, I am not sure if it actually fixes your issue, but it is worth a try. I found that you can actually keep the socket open from the outside (e.g. using curl) and I am unsure whether that is something caused by the Windows Kernel or by the fineftp server library. So maybe while trying to fix your issue I fixed an unrelated bug. Let's see.

Either way, here is a Powershell command that shows all open ports for an app (here fineftp_example):

Get-NetTCPConnection | Where-Object { (Get-Process | Where-Object { $_.Name -eq 'fineftp_example' }).Id -eq $_.OwningProcess }
monsieurgustav commented 1 day ago

Hi Florian, I'm afraid it does not fix my issue. It also crashes in release. When running release with debug symbols, I get this logs:

onecoreuap\internal\shell\inc\SrcPkg\FileExplorerSessionWatcher\inc\FileExplorerSessionWatcher.h(696)\SHELL32.dll!00007FFB133FE944: (caller: 00007FFB13343122) ReturnHr(1) tid(b988) 80004001 Non implémenté
shell\SrcPkg\FileExplorer\DefView\src\DefView.cpp(17508)\SHELL32.dll!00007FFB13343141: (caller: 00007FFADC25429E) LogHr(1) tid(b988) 80004001 Non implémenté
onecoreuap\internal\shell\inc\SrcPkg\FileExplorerSessionWatcher\inc\FileExplorerSessionWatcher.h(696)\SHELL32.dll!00007FFB133FE944: (caller: 00007FFB13343122) ReturnHr(2) tid(b988) 80004001 Non implémenté
shell\SrcPkg\FileExplorer\DefView\src\DefView.cpp(17508)\SHELL32.dll!00007FFB13343141: (caller: 00007FFADC25429E) LogHr(2) tid(b988) 80004001 Non implémenté
HEAP[BackdropLauncher.exe]: Heap block at 000001EAA9EEBF60 modified at 000001EAA9EEBF78 past requested size of 8

Unless you see another benefit, I suggest leaving this fix out.

monsieurgustav commented 1 day ago

Based on this discussion, I tried something else:

    void restartFtpServer(std::unique_ptr<fineftp::FtpServer>& ftpServer, const QString& folder, uint16_t port, size_t threadCount = 1)
    {
        if (ftpServer)
        {
            ftpServer->stop();
            ftpServer.reset();
        }

        QTimer::singleShot(0, [&ftpServer, folder, port, threadCount] {
            ftpServer = std::make_unique<fineftp::FtpServer>(port);
            ftpServer->addUserAnonymous(folder.toUtf8().toStdString(), fineftp::Permission::ReadOnly);
            ftpServer->start(threadCount);
        });
    }

Just in case you don't read Qt code: I wait an event loop iteration between the destruction and the re-creation of the server. And that works apparently... The FTP client reconnects automatically to the new server, adjusting the root folder as expected.

It's not ideal, I'd like to know how it is different, but I also don't care that much and I can live with it!

So I close the issue, feel free to re-open it if you want to investigate more.

FlorianReimold commented 10 hours ago

I am still investigating. I used a lot of Qt in the past and still maintain Qt projects, so I know what you are doing there. I just translated your restartFtpServer function in plain C++ so I don't need to create a Qt project.

Maybe you can answer a few questions, as I totally fail to reproduce your issue (Wrote a test that restarts the server 1000 times and accessed it with CURL and WinSCP in parallel, still no issue):

I pushed another commit to the same branch. This time I didn't aim to fix the issue but actually make it worse. Before, fineFTP opened sockets shared, so multiple server could be opened on the same port (which doesn't really make sense). I removed that code, so now you should be confronted with the following error message, if fineFTP is supposed to open a port that is already in use: Error binding acceptor: Typically, each socket address (protocol, network address, or port) may only be used once.