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

Add error handling and enable graceful shutdown #2131

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

Conversation

IronJam11
Copy link
Contributor

PR Description:

Closes #1350

Deliverables:

  • Adds logging for errors encountered in the handle() function.
  • Ensures graceful shutdown of the connection in the event of an error.

Changes:

  • Error Handling in handle():

    • Added a try-except block to catch exceptions during the handle() function execution.
    • Logs the error with a detailed traceback using logger.error().
    • Sends a 500 Internal Server Error response using self.send_response() when an error occurs.
    except Exception:
        logger.error(f"Error in handle(): {traceback.format_exc()}")
        await self.send_response(500, b"Internal Server Error")
        raise
  • Graceful Shutdown:

    • Ensured that after sending the error response, the connection is gracefully terminated using self.send_response().
  • Improved Error Handling in http_disconnect():

    • Added robust error handling during the disconnect phase to ensure proper cleanup and termination of the connection.

Comment on lines 10 to 18
if not logger.hasHandlers():
handler = logging.StreamHandler()
formatter = logging.Formatter(
"%(asctime)s - %(name)s - %(levelname)s - %(message)s"
)
handler.setFormatter(formatter)
logger.addHandler(handler)
logger.setLevel(logging.DEBUG)

Copy link
Member

Choose a reason for hiding this comment

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

Adding the logging call is fine I guess, but I don't think we should be adding handlers for users. That's up to them.

Comment on lines 20 to 23

Note that the ASGI spec requires that the protocol server only starts
sending the response to the client after ``self.send_body`` has been
called the first time.
Copy link
Member

Choose a reason for hiding this comment

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

Can you undo all of these unrelated changes to comments please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am so sorry about these

if not message.get("more_body"):
try:
await self.handle(b"".join(self.body))
except Exception:
logger.error(f"Error in handle(): {traceback.format_exc()}")
await self.send_response(500, b"Internal Server Error")
Copy link
Member

Choose a reason for hiding this comment

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

So this is the essence of the proposal.

Comment on lines 97 to 103
try:
await self.disconnect()
await aclose_old_connections()
except Exception as e:
logger.error(f"Error during disconnect: {str(e)}")
finally:
raise StopConsumer()
Copy link
Member

Choose a reason for hiding this comment

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

This is an extra.

@IronJam11
Copy link
Contributor Author

Made the changes as requested

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Thanks @IronJam11. I need to test this against an actual application, so give me a cycle.

if not message.get("more_body"):
try:
await self.handle(b"".join(self.body))
except Exception:
logger.error(f"Error in handle(): {traceback.format_exc()}")
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about using logger.exception() ?

@IronJam11
Copy link
Contributor Author

I was a bit caught up with a hackathon. apologies for the wait.

@carltongibson
Copy link
Member

No problem. No rush. I need to find a cycle to sit down with this properly. 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection won't be closed if raise exception in AsyncHttpConsumer::handle()
2 participants