Don't completely crash server on uncaught exception #228
Conversation
src/hypercorn/trio/tcp_server.py
Outdated
| @@ -48,28 +49,31 @@ async def run(self) -> None: | |||
| socket = self.stream.socket | |||
| ssl = False | |||
|
|
|||
| def log_handler(e: Exception) -> None: | |||
| if self.config.log.error_logger is not None: | |||
| self.config.log.error_logger.exception("Internal hypercorn error") | |||
There was a problem hiding this comment.
I think you want error_logger.error("Internal hypercorn error", exc_info=e), because the exception being passed in might only be a slice of the exception group that was being caught, so the implicit "pull the current exception from ambient context" logic in error_logger.exception might not grab the right data
There was a problem hiding this comment.
good call, done!
| await self._start_idle() | ||
| await self._read_data() | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
There's a minor behavioral change here where we used to not log OSErrors, but now we do. Probably it's better to keep that, because OSErrors are probably like "the other side hung up" or similar and not interesting? I think you can do it with like
catch({OSError: ignore_handler, Exception: log_handler})
|
@pgjones we just discovered the hard way that you reverted this... did you come up with the better solution you were looking for? Or could we un-revert it until you have a chance to replace with the proper fix? |
Partial mitigation for #225
This doesn't fix the underlying
AttributeError: 'WSStream' object has no attribute 'connection'but it does allow the server to stay up and healthy when it encounters this bug.