Asynchronously transfer files via ftp

[Edgaru089]

Is it possible for sf::Ftp to transfer(download/upload) files in a separate thread?
Something like this:

sf::Ftp ftp;
ftp.connect("ftp.server.com");
ftp.login("username","password");

ftp.startDownload("remotefile.txt", "local/path");

while(ftp.isTransfering())
    std::cout<<"\rStatus: "<<ftp.getTransferPercentage()<<"%";
std::cout<<std::endl<<"Done."<<std::endl;
 

[Laurent]

Asynchronous FTP transfers has been planned for ages... but since it's not a critical component of SFML, nobody ever started working on it. Have you tried to perform the download in a thread? You won't get any feedback (progress), but at least it won't block your main thread.

[Edgaru089]

I forked the SFML git repository a few months ago (https://github.com/Edgaru089/SFML), and I'm currently working on it. Should I start a issue first? Or just simply start a pull request when I'm ready?
BTW, should I use std::thread or sf::Thread to perform a transfer?

[Laurent]

I forked the SFML git repository a few months ago, and I'm currently working on it.

That's great

Should I start a issue first? Or just simply start a pull request when I'm ready?

We like to discuss things before they get implemented, so that we can agree on the public API before writing any code. You can also submit a finalized implementation if you prefer, but then there's the possibility that you end up rewriting parts of it after our review

For example, I wouldn't implement it the way you proposed in your first post, as it cannot handle multiple transfers at the same time; why implement parallel transfers if at the end you can only perform one at a time?

BTW, should I use std::thread or sf::Thread to perform a transfer?

Since SFML 2 is still stuck at C++03, use sf::Thread please.

[Edgaru089]

Since SFML 2 is still stuck at C++03, use sf::Thread please.

Ouch. I think that's one of the major reasons why nobody ever started working on it.

I just made&pushed a implementation using std::thread:

sf::Ftp::TransferState state;
server.downloadAsynced(filename, directory,sf::Ftp::Binary,state);

while (!state.isTransferCompleted())
    std::printf("\r%.2f KB / Total %.2f KB", (float)state.getTransferedSize() / 1024.0f, (float)fileSize / 1024.0f);

std::printf("\nCompleted", (float)fileSize / 1024.0f, (float)fileSize / 1024.0f);
 

[korczurekk]

Ftp::TransferState::m_pointerToTransferedSize seems suspicious to me.
Is your code thread safe? I see a few possible data races in Ftp::downloadAsynced, Ftp::download, Ftp::TransferState::getTransferedSize and Ftp::TransferState::isTransferCompleted. All of them use said m_pointerToTransferedSize without any synchronization.
While it will probably work, it looks like an undefined behavior to me. No idea how I'd fix it tho, mutex is an overkill and neither SFML nor C++03 provide atomics.

[Edgaru089]

I think this is thread safe, for only one thread would write data at once, and there's only one primitive data object (thus one memory block). The other threads are only reading the data, so there should be no problem with thread safety.

[korczurekk]

Well, I guess it's not a problem with types so small, that reading in the exact moment of writing is virtually impossible.

[Passer By]

Sorry for necroing something a billion years old.

I think this is thread safe, for only one thread would write data at once, and there's only one primitive data object (thus one memory block). The other threads are only reading the data, so there should be no problem with thread safety.

Writing and reading at the same time is a data race, and it's UB. There is no way around having atomics for flags, and that probably means relying on non-standard sources for them before C++11.

Looking at https://github.com/Edgaru089/SFML/blob/master/src/SFML/Network/Ftp.cpp.

void Ftp::downloadAsynced(const std::string & remoteFile, const std::string& localPath, TransferMode mode, TransferState& state)
{
    std::thread downloadThread(&Ftp::download, this, remoteFile, localPath, mode, std::ref(state));
    downloadThread.detach();

    // Wait until the transfer has started
    while(state.isTransferCompleted())
        sleep(milliseconds(5));
}
 

Can be legally optimized by the compiler into

void Ftp::downloadAsynced(const std::string & remoteFile, const std::string& localPath, TransferMode mode, TransferState& state)
{
    std::thread downloadThread(&Ftp::download, this, remoteFile, localPath, mode, std::ref(state));
    downloadThread.detach();

    // isTransferCompleted has no optimization barriers in it
    if(state.isTransferCompleted())
        while(true)
            sleep(milliseconds(5));
}
 

This isn't all data races can do to wreak your program, don't do it.

PS: gcc 7.2 does indeed make that optimization under O2