Skip to content
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.

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

fix connection exception cause high cpu load #1484

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dualc
Copy link

@dualc dualc commented Dec 18, 2024

fix #1483

@@ -147,6 +147,9 @@ def handle_incoming_message(self, message: str) -> None:
"""Send message to gateway server."""
if self.ws is None and self.ws_future is not None:
loop = IOLoop.current()
if self.ws_future.done():
self.log.error(f"Exception connect to gateway server {self.ws_future.exception()}")
Copy link
Contributor

@minrk minrk Dec 19, 2024

Choose a reason for hiding this comment

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

future.done() does not imply that it failed, whereas log message assumes it has.

Maybe the fix here is to add and not self.ws_future.done() to the condition on line 148? Or more lkely, the ws future error was already logged, and the right thing to do here is a debug (or warning, I'm not sure)-log for "message on closed websocket"?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah,you are right,I adjust it

Copy link
Author

Choose a reason for hiding this comment

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

maybe like this

            if self.ws_future.done() and isinstance(self.ws_future.exception(), Exception):
                self.log.error(f"Exception connect to server {self.ws_future.exception()}")
                return

if use if self.ws is None and not self.ws_future.done() when raise connection err,code will execute else logic self._write_message(message)

@@ -146,6 +146,9 @@ def handle_outgoing_message(self, incoming_msg: str, *args: Any) -> None:
def handle_incoming_message(self, message: str) -> None:
"""Send message to gateway server."""
if self.ws is None and self.ws_future is not None:
if self.ws_future.done() and isinstance(self.ws_future.exception(), Exception):
self.log.warning(f"Exception connect to websocket {self.ws_future.exception()}")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.log.warning(f"Exception connect to websocket {self.ws_future.exception()}")
self.log.warning(f"Exception connecting to websocket {self.ws_future.exception()}")

Comment on lines +149 to +150
if self.ws_future.done() and isinstance(self.ws_future.exception(), Exception):
self.log.warning(f"Exception connect to websocket {self.ws_future.exception()}")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should only log the Exception here if an exception in ws_future wouldn't already be logged, but it is. How about a simpler:

Suggested change
if self.ws_future.done() and isinstance(self.ws_future.exception(), Exception):
self.log.warning(f"Exception connect to websocket {self.ws_future.exception()}")
if self.ws_future.done() and self.ws_future.exception() is not None:
self.log.warning("Ignoring message on failed connection to kernel %s", self.kernel_id)

@minrk minrk added the bug label Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The gateway connection bug causes high CPU load
3 participants