Enabled logging of failed port bind (v2) by jvanasco · Pull Request #490 · Pylons/waitress

jvanasco · 2026-03-23T18:32:26Z

This PR enables logging on failed port binds. See #471

This is a continuation of #489 ; my git checkout got messed up. (While fixing some artifacts from merging to main, I accidentally named a new working branch from origin/feature-log_bind_failures instead of origin's feature-log_bind_failures)

…` to aid in debugging application bind failures. See Pylons#471

kgaughan

Some immediate comments. I need to take a look over a few other things too though.

src/waitress/wasyncore.py

tests/test_server.py

kgaughan

Almost there! There's some bit of leakage between the tests due to the redundant handling of self.inst and self.insts: by getting everything to use just self.insts, We should end up with everything properly cleaned up between tests. Hopefully I haven't missed anything!

kgaughan · 2026-03-23T20:18:23Z

tests/test_server.py

+        # most tests only start a single server
        if self.inst is not None:
            self.inst.close()



I don't think you need this: the for loop should already be handling that if you update _makeOneWithMulti and _makeOneWithSockets to push onto self.insts. Also, it's potentially closing at least one of the instances twice as the last instance in self.insts created by self._makeOne will be the same as what's in self.inst.

kgaughan · 2026-03-23T20:31:02Z

tests/test_server.py

I think we only really need self.insts, so...

Suggested change

inst = create_server(

kgaughan · 2026-03-23T20:34:02Z

tests/test_server.py

+        self.insts.append(self.inst)
        return self.inst


...and:

Suggested change

self.insts.append(self.inst)

return self.inst

self.insts.append(inst)

return inst

Nothing outside of the constructor, tearDown, and the _makeOne* methods need to access any of this, so using self.insts for everything keeps cleanup more straighforward.

kgaughan · 2026-03-23T20:34:52Z

tests/test_server.py

Same again:

Suggested change

inst = create_server(

kgaughan · 2026-03-23T20:35:19Z

tests/test_server.py

Suggested change

self.insts.append(inst)

return inst

kgaughan · 2026-03-23T20:35:31Z

tests/test_server.py

Suggested change

inst = create_server(

kgaughan · 2026-03-23T20:36:00Z

tests/test_server.py

@@ -91,9 +110,15 @@ def _makeWithSockets(
        return self.inst


Suggested change

return self.inst

self.insts.append(inst)

return inst

kgaughan · 2026-03-23T20:37:35Z

tests/test_server.py

+        if self.insts:
+            for inst in self.insts:
+                inst.close()


This if statement is redundant: the for loop won't run its contents if there's nothing in the list.

Doing self.insts = [] after the loop to prevent bleed-over between tests.

Suggested change

if self.insts:

for inst in self.insts:

inst.close()

for inst in self.insts:

inst.close()

self.insts = []

kgaughan · 2026-03-23T20:37:53Z

tests/test_server.py

+                inst.close()
+
    def test_ctor_app_is_None(self):
        self.inst = None


Suggested change

self.inst = None

self.insts = []

We can just drop this line, as the init sets this up already.

kgaughan · 2026-03-23T20:39:05Z

tests/test_server.py

+    # most tests only start a single server (for cleanup)
+    inst: Union["BaseWSGIServer", "MultiSocketServer", None]
+


With the rest of the suggested changes, this is now redundant.

jvanasco · 2026-03-23T20:46:40Z

Ok. I'm absolutely down to replace everything with self.insts .

I'll get through these on my next free block today.

kgaughan · 2026-03-23T20:51:59Z

tests/test_server.py

Ah! Spotted one place. Using self.inst here doesn't even really make sense.

Suggested change

inst = WSGIServer(None, _start=False, port=1234)

try:

# Ensure the adjustment was actually applied.

self.assertNotEqual(Adjustments.port, 1234)

self.assertEqual(inst.adj.port, 1234)

finally:

inst.close()

jvanasco · 2026-03-23T21:40:22Z

I'm trying to figure out where/how there is an unclosed socket in warnings.

jvanasco · 2026-03-23T21:57:10Z

Ok, the leak is due to #480. I thought this test case could be used to surface it the other day, and I was right.

When creating inst_c fails :

        # a third app should fail the bind to the fist app's host+port
        with self.assertLogs("waitress", level="ERROR") as cm_log:
            with self.assertRaises(OSError) as cm:
                # this line will trigger #480 and leave a socket open
                inst_c = self._makeOne(port=8080)
            self.assertEqual(cm.exception.errno, errno.EADDRINUSE)

it is because the logic in create_server doesn't allow for a proper cleanup.

I added some docstrings to this test; they can be cross-referenced into the other ticket.

I think a future PR to fix that behavior could leverage this test.

jvanasco added 2 commits March 20, 2026 16:29

Enabled logging of failed port bind in `waysyncore.py:dispatcher.bind…

face133

…` to aid in debugging application bind failures. See Pylons#471

cloud tests for some changes

698cfc8

kgaughan reviewed Mar 23, 2026

View reviewed changes

src/waitress/wasyncore.py Outdated Show resolved Hide resolved

tests/test_server.py Outdated Show resolved Hide resolved

addressed comments

3dc565e

kgaughan reviewed Mar 23, 2026

View reviewed changes

kgaughan linked an issue Mar 23, 2026 that may be closed by this pull request

Log port on failed bind? #471

Open

jvanasco added 2 commits March 23, 2026 17:30

test harness updates

b051c01

I think there was a bump on the linter version

bb58cd2

found the socket issue

a0e41df

		# most tests only start a single server (for cleanup)
		inst: Union["BaseWSGIServer", "MultiSocketServer", None]

+        inst = WSGIServer(None, _start=False, port=1234)
+        try:
+            # Ensure the adjustment was actually applied.
+            self.assertNotEqual(Adjustments.port, 1234)
+            self.assertEqual(inst.adj.port, 1234)
+        finally:
+            inst.close()

Conversation

jvanasco commented Mar 23, 2026

Uh oh!

kgaughan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kgaughan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvanasco commented Mar 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvanasco commented Mar 23, 2026

Uh oh!

jvanasco commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants