Conversation
…` to aid in debugging application bind failures. See Pylons#471
| return self.socket.listen(num) | ||
|
|
||
| def bind(self, addr): | ||
| # self.logger.log(logging.DEBUG, "Attempting to bind to: %s" % addr) |
There was a problem hiding this comment.
| # self.logger.log(logging.DEBUG, "Attempting to bind to: %s" % addr) |
There was a problem hiding this comment.
Whoops! I'm sorry I left that in.
| self.logger.log(logging.CRITICAL, "Failed bind to: %s" % str(addr)) | ||
| self.logger.log(logging.CRITICAL, "Exception raised: %s" % str(exc)) |
There was a problem hiding this comment.
self.logger is just an instance of logging.Logger, so this would be preferable:
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| self.assertTrue( | ||
| ("[Errno 48] Address already in use" == str(cm.exception)) | ||
| or ("[Errno 98] Address already in use" == str(cm.exception)) | ||
| ) |
There was a problem hiding this comment.
Something like this is preferable:
| 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.
There was a problem hiding this comment.
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)", |
There was a problem hiding this comment.
Based on my previous suggestions:
| "CRITICAL:waitress:Failed bind to: ('127.0.0.1', 8080)", | |
| "CRITICAL:waitress:Failed bind to 127.0.0.1:8080", |
| # 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks. Will do. I'm not very familiar with waitress, and other pyramid projects that use this concept don't have this concern.
There was a problem hiding this comment.
| "CRITICAL:waitress:Failed bind to: ('127.0.0.1', 8080)", | ||
| cm_log.output, | ||
| ) | ||
| self.assertTrue( |
There was a problem hiding this comment.
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.
| @unittest.skipIf( | ||
| sys.platform.startswith("win"), "This test is not supported on Windows" | ||
| ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, I just re-read the description. Maybe something like this would be better:
| @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", | |
| ) |
|
I'm going to have to close this PR and open a new one. My local repo was wedged from an earlier git-merge, and a fresh checkout+sync did something weird to my github branch. |
This PR enables logging on failed port binds. See #471
There is a test, but it does not run on windows.
After doing a lot of testing on the Github CI system, Windows does not appear to raise an exception here (though it should, according to docs.)
Note the tests run against two possible OsError codes. Either 48 or 98 is raised, depending on the python version and os.
The logging was easy. Getting the testing right, and merging against main was not ;)