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

added safety for missing keys in WebSocketDisconnect raise #2539

Closed
wants to merge 1 commit into from

Conversation

m9e
Copy link

@m9e m9e commented Mar 13, 2024

Summary

Saw issues in an app using FastAPI like:

  File "/Users/matt/code/agentzero/venv/lib/python3.10/site-packages/starlette/websockets.py", line 105, in _raise_on_disconnect
    raise WebSocketDisconnect(message["code"])
KeyError: 'code'

Discovered that while I was catching starlette.WebSocketDisconnect, it lacked a 'code' field which was throwing a KeyError which also had to be caught.

eg:

        try:
            data = await websocket.receive_text()
            data_json = json.loads(data)  # Convert the received text to a JSON object
            user_input = data_json.get('user_input')  # Extract the user_input part
        except json.JSONDecodeError as e:
            logging.error(f"Error decoding JSON from WebSocket: {e}")
            continue  # Skip to the next iteration on error
        except WebSocketDisconnect as e:
            logging.info(f"WebSocket disconnected: {e}. Client likely closed the connection.")
            break
        except Exception as e:
            if str(e) == 'code':
                logging.info(f"WebSocket disconnected: starlette error code (code). Client likely closed the connection.")
                break

This should make the params safe.

Very likely the issue here: #2434

Checklist

  • [ x ] I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • [ n/a ] I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • [ n/a ] I've updated the documentation accordingly.

@Kludex
Copy link
Sponsor Member

Kludex commented Mar 13, 2024

Code is a mandatory field based on the ASGI specification. The issue is probably elsewhere.

Please create a discussion with an MRE.

@Kludex Kludex closed this Mar 13, 2024
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.

None yet

2 participants