Fix Firefox WebSocket crash by adding error handling to SocketHandler Issue#942 #943
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Uses numpy.ma.masked_invalid to ignore NaN values. Alternatively, np.nan_to_num replaces NaN with a default (e.g., 0).
Reviewer's GuideThis PR enhances server robustness by adding comprehensive error handling to WebSocket handlers—wrapping open, message, and close events in try/except blocks with logging and graceful socket closure—and extends the line plotting API to sanitize input data via NumPy conversion, NaN masking, and updated send logic with dynamic window naming. Sequence diagram for improved WebSocket error handling in SocketHandlersequenceDiagram
participant Client
participant SocketHandler
participant Server
Client->>SocketHandler: Open WebSocket
SocketHandler->>Server: Register client (try/catch)
Note over SocketHandler: Logs error if exception
Client->>SocketHandler: Send message
SocketHandler->>Server: Handle message (try/catch)
Note over SocketHandler: Logs error, closes socket on exception
Client-->>SocketHandler: Abrupt disconnect
SocketHandler->>Server: Cleanup (try/catch)
Note over SocketHandler: Logs closure event
Class diagram for updated SocketHandler error handlingclassDiagram
class SocketHandler {
+check_origin(origin)
+open()
+on_message(message)
+on_close()
}
SocketHandler : +try/catch in open()
SocketHandler : +try/catch in on_message()
SocketHandler : +try/catch in on_close()
SocketHandler : +logging
SocketHandler : +self.close() on error
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Joy-chakraborty-23 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Allowing all origins in check_origin may introduce security risks. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `py/visdom/__init__.py:1815` </location>
<code_context>
labels = np.tile(labels, (Y.shape[0], 1)).ravel(order="F")
+ win = 'line_' + env + str(idx)
+ return _send({
+ 'data': Y.tolist(), # Send cleaned data
+ 'win': win,
+ 'eid': env,
+ 'layout': layout,
+ 'opts': opts})
+
return self.scatter(
</code_context>
<issue_to_address>
Early return from the function bypasses the original scatter logic.
If the early return is intentional, remove the unreachable scatter logic below. Otherwise, clarify when each path should be used.
</issue_to_address>
### Comment 2
<location> `py/visdom/server/app.py:221` </location>
<code_context>
return settings
+class SocketHandler(tornado.websocket.WebSocketHandler):
+ def check_origin(self, origin):
+ return True # Or customize for security
+
+ def open(self):
</code_context>
<issue_to_address>
Allowing all origins in check_origin may introduce security risks.
Disabling origin checks exposes the WebSocket to CSRF and similar attacks. Restrict allowed origins or make this configurable for production.
</issue_to_address>
### Comment 3
<location> `py/visdom/server/app.py:231` </location>
<code_context>
+ except Exception as e:
+ logging.error(f"WebSocket open error: {e}")
+
+ def on_message(self, message):
+ try:
+ # Existing message handling logic
+ pass
+ except Exception as e:
+ logging.error(f"WebSocket message error: {e}")
+ self.close() # Gracefully close on error
+
+ def on_close(self):
</code_context>
<issue_to_address>
Closing the WebSocket on any message error may be overly aggressive.
Consider handling exceptions more selectively, logging non-critical errors, and only closing the connection for serious issues.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def on_message(self, message):
try:
# Existing message handling logic
pass
except Exception as e:
logging.error(f"WebSocket message error: {e}")
self.close() # Gracefully close on error
=======
class CriticalWebSocketError(Exception):
"""Exception for critical WebSocket errors that require closing the connection."""
pass
def on_message(self, message):
try:
# Existing message handling logic
pass
except CriticalWebSocketError as e:
logging.error(f"Critical WebSocket message error: {e}")
self.close() # Gracefully close on critical error
except Exception as e:
logging.warning(f"Non-critical WebSocket message error: {e}")
# Optionally, send an error message to the client or take other action
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1815
to
+1820
| return _send({ | ||
| 'data': Y.tolist(), # Send cleaned data | ||
| 'win': win, | ||
| 'eid': env, | ||
| 'layout': layout, | ||
| 'opts': opts}) |
There was a problem hiding this comment.
question: Early return from the function bypasses the original scatter logic.
If the early return is intentional, remove the unreachable scatter logic below. Otherwise, clarify when each path should be used.
Comment on lines
+221
to
+222
| def check_origin(self, origin): | ||
| return True # Or customize for security |
There was a problem hiding this comment.
🚨 issue (security): Allowing all origins in check_origin may introduce security risks.
Disabling origin checks exposes the WebSocket to CSRF and similar attacks. Restrict allowed origins or make this configurable for production.
Comment on lines
+231
to
+237
| def on_message(self, message): | ||
| try: | ||
| # Existing message handling logic | ||
| pass | ||
| except Exception as e: | ||
| logging.error(f"WebSocket message error: {e}") | ||
| self.close() # Gracefully close on error |
There was a problem hiding this comment.
suggestion: Closing the WebSocket on any message error may be overly aggressive.
Consider handling exceptions more selectively, logging non-critical errors, and only closing the connection for serious issues.
Suggested change
| def on_message(self, message): | |
| try: | |
| # Existing message handling logic | |
| pass | |
| except Exception as e: | |
| logging.error(f"WebSocket message error: {e}") | |
| self.close() # Gracefully close on error | |
| class CriticalWebSocketError(Exception): | |
| """Exception for critical WebSocket errors that require closing the connection.""" | |
| pass | |
| def on_message(self, message): | |
| try: | |
| # Existing message handling logic | |
| pass | |
| except CriticalWebSocketError as e: | |
| logging.error(f"Critical WebSocket message error: {e}") | |
| self.close() # Gracefully close on critical error | |
| except Exception as e: | |
| logging.warning(f"Non-critical WebSocket message error: {e}") | |
| # Optionally, send an error message to the client or take other action |
|
@Joy-chakraborty-23 Thanks for looking into this. Please respond to sourcery-ai. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description:
Issue: Firefox crashes the Visdom server due to unhandled WebSocket disconnections (#942).
Root Cause:
SocketHandlerandVisSocketHandlerinsocket_handlers.pylack error handling for abrupt WebSocket closures (common in Firefox).Changes:
try-catchblocks insocket_handlers.pyto:Impact:
Steps to Test (Offline Workflow):
main()inrun_server.py:python run_server.pyafter patching.Files Modified:
Key Technical Fixes :
SocketHandlerandVisSocketHandlerinsocket_handlers.py.on_message(),open(), andon_close()with error logging.self.close()on exceptions to prevent orphaned sockets.Summary by Sourcery
Add error handling to WebSocket handlers to prevent server crashes on abrupt disconnects and improve the line plotting API by sanitizing NaN values and refining the data payload.
Bug Fixes:
Enhancements: