Skip to content

Enabled logging of failed port bind #489

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.

Sign up for GitHub

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

Closed
jvanasco wants to merge 1 commit into from
+49 −1
Closed

Enabled logging of failed port bind #489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ Bugfix
https://github.com/Pylons/waitress/pull/475 and
https://github.com/Pylons/waitress/issues/464

- Enabled logging of failed port bind in `waysyncore.py:dispatcher.bind` to aid
in debugging application bind failures. See
https://github.com/Pylons/waitress/issues/471


3.0.2 (2024-11-16)
------------------

Expand Down
8 changes: 7 additions & 1 deletion src/waitress/wasyncore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,8 +370,14 @@ def listen(self, num):
return self.socket.listen(num)

def bind(self, addr):
# self.logger.log(logging.DEBUG, "Attempting to bind to: %s" % addr)
Copy link
Copy Markdown
Member

@kgaughan kgaughan Mar 21, 2026

Choose a reason for hiding this comment

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

Suggested change
# self.logger.log(logging.DEBUG, "Attempting to bind to: %s" % addr)

Copy link
Copy Markdown
Contributor Author

@jvanasco jvanasco Mar 21, 2026

Choose a reason for hiding this comment

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

Whoops! I'm sorry I left that in.

self.addr = addr
return self.socket.bind(addr)
try:
return self.socket.bind(addr)
except Exception as exc:
self.logger.log(logging.CRITICAL, "Failed bind to: %s" % str(addr))
self.logger.log(logging.CRITICAL, "Exception raised: %s" % str(exc))
Comment on lines +378 to +379
Copy link
Copy Markdown
Member

@kgaughan kgaughan Mar 21, 2026

Choose a reason for hiding this comment

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

self.logger is just an instance of logging.Logger, so this would be preferable:

Suggested change
self.logger.log(logging.CRITICAL, "Failed bind to: %s" % str(addr))
self.logger.log(logging.CRITICAL, "Exception raised: %s" % str(exc))
host, port = addr
self.logger.critical("Failed bind to %s:%d: %s", host, port, exc)

Copy link
Copy Markdown
Contributor Author

@jvanasco jvanasco Mar 21, 2026

Choose a reason for hiding this comment

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

I prefer invoking logging like that as well; I was just going with the existing usage in the rest of the file.

Regarding host, port = addr – is this guaranteed to be %s/%d?? Waitress doesn't have inline typing, and I was worried about how different connection configurations would play out.

Copy link
Copy Markdown
Member

@kgaughan kgaughan Mar 21, 2026

Choose a reason for hiding this comment

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

A good point. It depends on the address family, so as Waitress supports AF_INET, AF_INET6, and AF_UNIX, it should be able to cope with AF_INET6 properly, so maybe something more like this:

if self.socket.family == socket.AF_INET:
    self.logger.critical("Failed bind to %s:%d: %s", addr[0], addr[1], exc)
elif self.socket.family == socket.AF_INET6:
    self.logger.critical("Failed bind to [%s]:%d: %s", addr[0], addr[1], exc)
else:
    self.logger.critical("Failed bind to %s: %s", addr, exc)

That'll handle IPv4 and IPv6 addresses, I think, though really some kind of proper formatting function would be better. I'll leave it up to you as to whether you just want to format the address with%s or format it based on the family. I don't think there's any standard library function for doing anything like that.

Copy link
Copy Markdown
Contributor Author

@jvanasco jvanasco Mar 21, 2026

Choose a reason for hiding this comment

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

Yeah, the ipv4 & unix socket was why I just used a single string representation to start with. i did not think of ipv6, which complicates things even more.

I think this might be a good option:

self.logger.critical("Failed bind to `%s`: %s", addr, exc)

The addr will be in the backticks, the exc is after. That should be clear for humans and is easy to read/process by machine.

I've got a block of time tomorrow to play with this more.

raise

def accept(self):
# XXX can return either an address pair or None
Expand Down
37 changes: 37 additions & 0 deletions tests/test_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import errno
import socket
import sys
import unittest

dummy_app = object()
Expand Down Expand Up @@ -311,6 +312,42 @@ def test_create_with_one_socket_handle_accept_noerror(self):
self.assertListEqual(innersock.opts, [("level", "optname", "value")])
self.assertListEqual(L, [(inst, innersock, None, inst.adj)])

@unittest.skipIf(
sys.platform.startswith("win"), "This test is not supported on Windows"
)
Comment on lines +315 to +317
Copy link
Copy Markdown
Member

@kgaughan kgaughan Mar 21, 2026

Choose a reason for hiding this comment

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

How is it breaking on Windows. I see nothing here that shouldn't work.

My suggestions might help fix whatever issue was causing issues on Windows, but if they don't, it'd be better to say why it doesn't work on Windows rather than just stating that it doesn't.

Copy link
Copy Markdown
Member

@kgaughan kgaughan Mar 21, 2026

Choose a reason for hiding this comment

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

Ah, I just re-read the description. Maybe something like this would be better:

Suggested change
@unittest.skipIf(
sys.platform.startswith("win"), "This test is not supported on Windows"
)
@unittest.skipIf(
sys.platform.startswith("win"),
"Windows doesn't raise an exception when reusing a port",
)

Copy link
Copy Markdown
Contributor Author

@jvanasco jvanasco Mar 21, 2026

Choose a reason for hiding this comment

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

agree

def test_port_bind_failure_logging(self):
# ensure the address is logged on a failed port bind

# create a first app correctly
inst_a = self._makeOne(port=8080)

# Ensure a second app correctly binds to a different host+port
inst_b = self._makeOne(port=8081)
Comment on lines +321 to +325
Copy link
Copy Markdown
Member

@kgaughan kgaughan Mar 21, 2026

Choose a reason for hiding this comment

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

Using self._makeOne multiple times in a test is problematic. If you look at it and the tearDown method, there's an assumption that it only gets called once per test. That means you're leaking sockets. You'll have to update _makeOne to collect any created in a list and tearDown to close the collected sockets and truncate the list.

Copy link
Copy Markdown
Contributor Author

@jvanasco jvanasco Mar 21, 2026

Choose a reason for hiding this comment

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

Thanks. Will do. I'm not very familiar with waitress, and other pyramid projects that use this concept don't have this concern.

Copy link
Copy Markdown
Contributor Author

@jvanasco jvanasco Mar 21, 2026

Choose a reason for hiding this comment

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

I got this ironed out quickly, HOWEVER the purposefully failed connection will leave a socket opened due to issue #480

I will leave a note in the test case and the #480 ticket about that - because that issue does not have a test case, and this can double for that.


# 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:
inst_c = self._makeOne(port=8080)
self.assertTrue(
("[Errno 48] Address already in use" == str(cm.exception))
or ("[Errno 98] Address already in use" == str(cm.exception))
)
Comment on lines +331 to +334
Copy link
Copy Markdown
Member

@kgaughan kgaughan Mar 21, 2026

Choose a reason for hiding this comment

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

Something like this is preferable:

Suggested change
self.assertTrue(
("[Errno 48] Address already in use" == str(cm.exception))
or ("[Errno 98] Address already in use" == str(cm.exception))
)
self.assertEqual(cm.exception.errno, errno.EADDRINUSE)

Using the named constants is more cross-platform.

Copy link
Copy Markdown
Contributor Author

@jvanasco jvanasco Mar 21, 2026

Choose a reason for hiding this comment

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

Thanks so much for this. I was going through docs and the matrix trying to figure out what was going on; using errno makes so much more sense.


self.assertIn(
"CRITICAL:waitress:Failed bind to: ('127.0.0.1', 8080)",
Copy link
Copy Markdown
Member

@kgaughan kgaughan Mar 21, 2026

Choose a reason for hiding this comment

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

Based on my previous suggestions:

Suggested change
"CRITICAL:waitress:Failed bind to: ('127.0.0.1', 8080)",
"CRITICAL:waitress:Failed bind to 127.0.0.1:8080",

cm_log.output,
)
self.assertTrue(
Copy link
Copy Markdown
Member

@kgaughan kgaughan Mar 21, 2026

Choose a reason for hiding this comment

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

This is a bit brittle. If you want a test like this, you should be using the errno module and os.strerror to construct what the message would be.

(
"CRITICAL:waitress:Exception raised: [Errno 48] Address already in use"
in cm_log.output
)
or (
"CRITICAL:waitress:Exception raised: [Errno 98] Address already in use"
in cm_log.output
)
)


if hasattr(socket, "AF_UNIX"):

Expand Down