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

Controller.start uses threading #160

Closed
samuelcolvin opened this issue Jan 20, 2019 · 9 comments · Fixed by #256
Closed

Controller.start uses threading #160

samuelcolvin opened this issue Jan 20, 2019 · 9 comments · Fixed by #256
Assignees
Labels
API enhancement in progress Someone has started working on the issue but no PR yet
Milestone

Comments

@samuelcolvin
Copy link
Member

self._thread = threading.Thread(target=self._run, args=(ready_event,))

It seems to be that Controller.start using Threading is not required and will lead to problems.

Like the asyncio http server starting an SMTP server should not involve threading, but just loop.run_forever().

@waynew
Copy link
Collaborator

waynew commented Jan 20, 2019

@samuelcolvin thanks for the report! Do you have some docs or examples that we could reference?

@samuelcolvin
Copy link
Member Author

Take a look at aiohttp's run_app or its documentation on Runnings Applications.

@pepoluan
Copy link
Collaborator

pepoluan commented Oct 22, 2020

@samuelcolvin so if I don't misunderstood, you want to split out the thread-starting ability from Controller?

Because currently all those code in Controller enables the event loop to run without blocking the main thread. If we take out the thread-creation part, then invoking run_forever() will block.

And this will be a major, very impactful breaking behavioral change for existing users.

EDIT: Rather than changing the existing Controller class, we can create a new class -- we can call it UnthreadedController or somesuch -- where the threading circuitry is taken out. So if user really wants to handle their own multithreading/multiprocessing management, they can use that class instead. Users already using existing Controller class don't have to do anything on their end.

@nhoad
Copy link

nhoad commented Dec 7, 2020

Why is the default behavior to run in a thread?

Mirroring Samuel's request, having the ability to run it in the foreground without any threading would be useful.

@pepoluan
Copy link
Collaborator

pepoluan commented Dec 8, 2020

@nhoad One thing to realize is that Controller is just a "convenience class" to instantiate the SMTP server.

The code for Controller is very well designed; if you need a non-threading, in-the-foreground solution, you can simply take the code within the Controller._run() method (that is, select the text, copy them, and paste in your code) and run it.

All the intelligence of the SMTP server happens in the aiosmtpd.smtp.SMTP class; it's self-contained.

@pepoluan
Copy link
Collaborator

pepoluan commented Feb 17, 2021

I have a branch on my laptop, currently waiting for #247 to be merged first because it's based on that.

@pepoluan pepoluan added enhancement API in progress Someone has started working on the issue but no PR yet labels Feb 17, 2021
@pepoluan
Copy link
Collaborator

Boy oh boy... apparently implementing unthreaded controllers is much more complex than I thought.

Testing them properly is a whole 'nother level.

@pepoluan
Copy link
Collaborator

If anyone wants to take a peek and/or kick the tires, be my guest: https://github.com/pepoluan/aiosmtpd/tree/unthreaded-controller

@pepoluan
Copy link
Collaborator

pepoluan commented Feb 27, 2021

The branch is about 99% complete, documentation also updated. Passes all tests on Windows and WSL (no time to test it on FreeBSD and OpenSUSE yet)

Feel free to check it out @samuelcolvin @nhoad

@pepoluan pepoluan self-assigned this Feb 27, 2021
@pepoluan pepoluan added this to the 1.5 milestone Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API enhancement in progress Someone has started working on the issue but no PR yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants