Tidying init #261

pepoluan · 2021-03-05T01:40:17Z

~~Depends on #256 , #259 , and #260 ; please merge them first.~~ All deps have been merged. We can go ahead with this one.

What do these changes do?

The __init__ method is very unwieldy. This PR makes it simpler by extracting some logic into properties.

This led to several improvements:

event_handler can be replaced "on-the-fly"
tls_context can be replaced "on-the-fly" and will automagically change require_tls accordingly
User can selectively overload logic in the properties; no longer have to copy > 100 lines of init code just to override one behavior 😉

Are there changes in behavior for the user?

None.

Users have to be aware though that some properties are more 'noisy' in logging.

Related issue number

Inspired partially by #253

Checklist

codecov · 2021-03-05T01:44:17Z

Codecov Report

Merging #261 (4bd08bb) into master (4bd08bb) will not change coverage.
The diff coverage is n/a.

❗ Current head 4bd08bb differs from pull request most recent head 5f3557c. Consider uploading reports for the commit 5f3557c to get more accurate results

@@            Coverage Diff            @@
##            master      #261   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines         1706      1706           
  Branches       310       310           
=========================================
  Hits          1706      1706

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pepoluan · 2021-03-05T03:52:41Z

Now that I think of it ... this should be merged ONLY after #260 has been merged.

That way, I can test for the behavior of the library when tls_context is being replaced totally. (Current tests are replacing to/from None, and replacing with a new SSLContext object but using the same cert-key pair.)

waynew · 2021-03-24T20:12:03Z

Nice. I'm digging at least the idea :)

pepoluan

Some comments for possibly interesting decisions

pepoluan · 2021-03-24T15:28:30Z

aiosmtpd/smtp.py

+    # Exposed states
+    session: Optional[Session] = None
+    envelope: Optional[Envelope] = None
+
+    # Protected states
+    _loop: asyncio.AbstractEventLoop = None
+    _event_handler: Any = None
+    _smtp_methods: Dict[str, SmtpMethodType] = {}
+    _handle_hooks: Dict[str, Callable] = None
+    _ehlo_hook_ver: Optional[str] = None
+    _handler_coroutine: Optional[asyncio.Task] = None
+    _timeout_handle: Optional[asyncio.TimerHandle] = None
+
+    _tls_context: Optional[Union[ssl.SSLContext, _Missing]] = MISSING
+    _req_starttls: bool = False
+    _tls_handshake_okay: bool = True
+    _tls_protocol: Optional[sslproto.SSLProtocol] = None
+    _original_transport: Optional[asyncio.BaseTransport] = None
+
+    _auth_mechs: Dict[str, _AuthMechAttr] = {}
+    _auth_excludes: Optional[Iterable[str]] = None
+    _authenticator: Optional[AuthenticatorType] = None
+    _auth_callback: Optional[AuthCallbackType] = None


Extracting all these states greatly simplifies the innards of __init__, making it much more readable and thus more maintainable.

pepoluan · 2021-03-24T15:29:17Z

aiosmtpd/smtp.py

+    @property
+    def loop(self) -> asyncio.AbstractEventLoop:
+        return self._loop


self.loop is now a getter-only property to prevent accidental replacement.

pepoluan · 2021-03-29T06:08:40Z

housekeep.py

+    env = dict(os.environ)
+    env["PYTHON_EXE"] = str(sys.executable)


Also record the Python executable in the env var dump

pepoluan · 2021-03-29T06:08:59Z

housekeep.py

+def docs_clean(verbose=False):
+    """Cleanup build/ to force sphinx-build to rebuild"""
+    buildpath = Path("build")
+    if not buildpath.exists():
+        print("Docs already cleaned.")
+        return
+    print("Removing build/ ...", end="")
+    deldir(buildpath, verbose)
+    if verbose:
+        print(flush=True)


Quick housekeeping to regen docs

pepoluan · 2021-03-29T06:10:51Z

aiosmtpd/smtp.py

+        if not self._auth_mechs:
+            self._auth_mechs = {}


It is important that we assign a new dict to this attrib, to prevent 'contamination' between SMTP class instantiations. (Which apparently happens for every new connection to the listening port -- something I just figured out after all these months 😅)

pepoluan · 2021-03-29T06:11:37Z

aiosmtpd/smtp.py

-        self._smtp_methods: Dict[str, Any] = {
-            m.replace("smtp_", ""): getattr(self, m)
-            for m in dir(self)
-            if m.startswith("smtp_")
-        }


Moving method scanning outside of init speeds up class instantiation.

pepoluan · 2021-03-29T06:12:25Z

aiosmtpd/smtp.py

-        # Get hooks & methods to significantly speedup getattr's
-        self._auth_methods: Dict[str, _AuthMechAttr] = {
-            getattr(
-                mfunc, "__auth_mechanism_name__",
-                mname.replace("auth_", "").replace("__", "-")
-            ): _AuthMechAttr(mfunc, obj is self)
-            for obj in (self, handler)
-            for mname, mfunc in inspect.getmembers(obj)
-            if mname.startswith("auth_")
-        }


Moving method scanning outside of init, especially for a scanning as complex as this, speeds up SMTP instantiation.

pepoluan · 2021-03-29T06:18:05Z

As soon as test passes on my testing systems, I'm undrafting this and let this marinate.

I might not be able to do code changes for several days ahead, due to me transitioning from "being employed" to "being a free agent" and needing to acquire a new laptop. I reckon up to a week of radio silence due to the Easter holidays. Hopefully not as long as that, depending on how soon the store can fulfill my order.

pepoluan · 2021-03-29T06:21:47Z

Nice. I'm digging at least the idea :)

A side effect is that SMTP instatiation is now faster 😉

waynew · 2022-11-25T17:09:43Z

@pepoluan I'm going to plan on taking a closer look at this one this week, too. Looks like there are a couple of conflicts, if you're able to tackle those.

Now the common *Controller classes (excluding Base*Controller classes) are all based on mixins.

Too many pragmas to add.

Solve Issue#257

"Live properties" means properties that, when changed, will immediately be effective. A side benefit is that we can move validation/parsing logic out of the oversized __init__. The following attributes have been converted: * `event_handler` -- now on-the-fly event handler replacement is possible. Automatically refreshes methods_auth. * `tls_context` -- now on-the-fly TLS Context replacement (or disablement) is possible. * `require_starttls` -- now automatically adjusts to whether tls_context is set or not. Setting this property only sets the "backing" bool value. * `methods_auth` -- read-only property containing authentication methods, automatically populated from handler.

* Many internal states moved out of __init__ * Type hint for _smtp_methods now more specific: * Define type alias "SmtpMethodType" * Define Protocol "HasSMTPAttribs", used by SmtpMethod * Access to _smtp_methods now via property Slightly slower, but this makes __init__ tidier * Simplify Call Limit mechanism * Now uses only one attrib: _call_limit * Change loop into "read only" property

pepoluan · 2023-01-17T11:33:18Z

@pepoluan I'm going to plan on taking a closer look at this one this week, too. Looks like there are a couple of conflicts, if you're able to tackle those.

Fixed the conflicts, rebase, and let's see...

pepoluan · 2023-01-17T11:42:43Z

Oh gosh it blew up! LOL

pepoluan · 2023-01-17T11:43:48Z

Heh seriously at this point I'll just close this PR, but grab the ideas and make a new PR based on the latest master.

There are just too many incremental changes since this PR was created, the proposed changes in this PR makes things go awry.

pepoluan added the tech-debt Things that needs to be tidied up to avoid being bitten in the future... label Mar 5, 2021

pepoluan added this to the 1.5 milestone Mar 5, 2021

pepoluan self-assigned this Mar 5, 2021

pepoluan force-pushed the tidy-init branch 4 times, most recently from 94b24db to 6eaef4e Compare March 8, 2021 14:22

pepoluan force-pushed the tidy-init branch 2 times, most recently from e7eff56 to f146e6b Compare March 24, 2021 03:54

pepoluan commented Mar 29, 2021

View reviewed changes

pepoluan marked this pull request as ready for review March 29, 2021 06:20

pepoluan added 13 commits January 17, 2023 18:17

Completely rewrite aiosmtpd.controller

6866170

Now the common *Controller classes (excluding Base*Controller classes) are all based on mixins.

Move autostop_loop and AUTOSTOP_DELAY to conftest.py

0e67a5f

Solve SIM* alerts

3dcdd7f

Disable SIM106

8b70aee

Too many pragmas to add.

Solve SIM* alerts

8860e53

Update NEWS.rst

7320904

Implement customizable local part limit

e48976b

Solve Issue#257

Test the changes in tls_context handling

457caf1

Reorder params and slight optimization in __init__

e1dd84e

Test __init__ changes

46a734c

Adapt test cases to changes in smtp.py

abfd75f

pepoluan added 10 commits January 17, 2023 18:27

Add some #pragma's

199a983

Solve qa complaints

f004fc2

Update docs

ece3263

Solve pytest complaint

c32f45c

Blackification

09b89e1

Blackify __init__

ca9fc1a

Fix rebase error

fc9ae18

Fix rebase errors

9b5bf7f

Implement test logging, from instrumentation-1 branch

1dd5276

Improve housekeep.py

5f3557c

pepoluan force-pushed the tidy-init branch from dc225e4 to 5f3557c Compare January 17, 2023 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidying init #261

Tidying init #261

pepoluan commented Mar 5, 2021 •

edited

codecov bot commented Mar 5, 2021 •

edited

pepoluan commented Mar 5, 2021 •

edited

waynew commented Mar 24, 2021

pepoluan left a comment

pepoluan Mar 24, 2021 •

edited

pepoluan Mar 24, 2021

pepoluan Mar 29, 2021

pepoluan Mar 29, 2021

pepoluan Mar 29, 2021 •

edited

pepoluan Mar 29, 2021

pepoluan Mar 29, 2021

pepoluan commented Mar 29, 2021

pepoluan commented Mar 29, 2021

waynew commented Nov 25, 2022

pepoluan commented Jan 17, 2023

pepoluan commented Jan 17, 2023

pepoluan commented Jan 17, 2023 •

edited

		env = dict(os.environ)
		env["PYTHON_EXE"] = str(sys.executable)

Tidying __init__ #261

Are you sure you want to change the base?

Tidying __init__ #261

Conversation

pepoluan commented Mar 5, 2021 • edited

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

codecov bot commented Mar 5, 2021 • edited

Codecov Report

pepoluan commented Mar 5, 2021 • edited

waynew commented Mar 24, 2021

pepoluan left a comment

Choose a reason for hiding this comment

pepoluan Mar 24, 2021 • edited

Choose a reason for hiding this comment

pepoluan Mar 24, 2021

Choose a reason for hiding this comment

pepoluan Mar 29, 2021

Choose a reason for hiding this comment

pepoluan Mar 29, 2021

Choose a reason for hiding this comment

pepoluan Mar 29, 2021 • edited

Choose a reason for hiding this comment

pepoluan Mar 29, 2021

Choose a reason for hiding this comment

pepoluan Mar 29, 2021

Choose a reason for hiding this comment

pepoluan commented Mar 29, 2021

pepoluan commented Mar 29, 2021

waynew commented Nov 25, 2022

pepoluan commented Jan 17, 2023

pepoluan commented Jan 17, 2023

pepoluan commented Jan 17, 2023 • edited

Tidying init #261

Tidying init #261

pepoluan commented Mar 5, 2021 •

edited

codecov bot commented Mar 5, 2021 •

edited

pepoluan commented Mar 5, 2021 •

edited

pepoluan Mar 24, 2021 •

edited

pepoluan Mar 29, 2021 •

edited

pepoluan commented Jan 17, 2023 •

edited