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

feat: support use with statement to create a ftpserver temporarily #567

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ramwin
Copy link

@ramwin ramwin commented Jan 21, 2022

No description provided.

@ramwin
Copy link
Author

ramwin commented Jan 22, 2022

@giampaolo the continuous-integration test failed. But the log shows that this is caused by pyftpdlib.test.test_servers.TestCornerCasesThreadMixin.test_quick_connect . I think it's not caused by my code. Could you review this pull request when you have spare time?

@ramwin
Copy link
Author

ramwin commented Jan 25, 2022

@giampaolo , Please review this pull request when you have any free time. Thanks a lot.

@gliptak
Copy link
Contributor

gliptak commented Dec 6, 2022

consider rerunning with current CI

@ramwin
Copy link
Author

ramwin commented Dec 12, 2022

Hello, @gliptak . I rebase my branch onto the master branch. Review this pull request again when you are free.

@giampaolo
Copy link
Owner

giampaolo commented Dec 12, 2022

I don't think this is necessary (or at least not in this form). Do you need this to have a test FTP server to connect to during unit tests? If that's the case, then I think it may make more sense to add __enter__ / __exit__ to the test FTP server class:

class ThreadedTestFTPd(threading.Thread):

return

def __exit__(self, exc_type, exc_value, traceback):
logger.debug("you are existing FTPServerContext")
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: existing -> exiting

@ramwin
Copy link
Author

ramwin commented Dec 13, 2022

I have a simple script that need to start a ftpserver and call other web service to connect the ftpserver. It was inconvinent to first start a ftpserver and than run the simple script. I want to achieve this using with statement

with FtpServer(ftpusername, ftppassword) as tmp_ftpserver:
    requests.post("http://otherserver/download", json={
        ftpusername, ftppassword, filepath
    })

after the script, the ftpserver can be closed automatically.

@ramwin
Copy link
Author

ramwin commented Dec 13, 2022

Other wise, I will have to open two terminal.

First terminal

python -m ftpdlib --username=admin --password

Second terminal

python3 main.py

and I can only stop the ftpserver after the main script finished.

@giampaolo
Copy link
Owner

I forgot I added it, but FTPServer class already supports the context manager protocol:
327f3e0
That is different than your context manager though: on __enter__ it does NOT automatically call server_forever() in a thread.

@ramwin
Copy link
Author

ramwin commented Dec 15, 2022

I forgot I added it, but FTPServer class already supports the context manager protocol: 327f3e0 That is different than your context manager though: on __enter__ it does NOT automatically call server_forever() in a thread.

The function automatically call server_forever() in a thread is what I really want. That's why I create the FTPServerContext. I want to create a ftpserver temporally and use it in afterword scripts.

@ramwin
Copy link
Author

ramwin commented Dec 15, 2022

I tried use the blocking=False parameter, It didn't work as I wanted. My code looks like this:

with FTPServer(("localhost", 2121), handler) as f:
    f.serve_forever(blocking=False)
    print("start success")
    with FTP() as ftp:
        ftp.connect(host='localhost', port=2121)
        ftp.login()
        print(ftp.dir())

there is no start success printed on the screen, neither will the later code been executed.

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