Fix/socket clean up#258
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Refactoring
PR Summary: The pull request introduces a deferred cleanup function within the HandleWebSocketForLogs method to ensure that resources associated with jetstream consumers are properly released when the function exits. This change aims to enhance the robustness and reliability of the websocket server's resource management.
Decision: Comment
📝 Type: 'Refactoring' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.
General suggestions:
- Consider enhancing the error logging within the deferred cleanup function to include more context-specific information. This could help in debugging by providing clearer insights into the circumstances under which resources fail to be released.
- Review the use of error formatting directives in logging statements. Ensure consistency and correctness in how errors are logged to avoid potential confusion during debugging.
- Verify the idempotency of the
Stopmethod on jetstream consumers. Since the cleanup function does not conditionally execute based on the state of the resources, ensuring thatStopcan be safely called multiple times or on already stopped consumers will prevent unintended side effects.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| defer func() { | ||
| for _, v := range resources { | ||
| if v.jc != nil { | ||
| if err := v.jc.Stop(ctx); err != nil { |
There was a problem hiding this comment.
suggestion (llm): Consider logging more context-specific information along with the error message. This could include identifiers or states relevant to the Subscription or jetstream consumer that failed to stop. It will make debugging easier by providing more insights into why the stop operation failed.
| for _, v := range resources { | ||
| if v.jc != nil { | ||
| if err := v.jc.Stop(ctx); err != nil { | ||
| log.Warnf("stop jetstream consumer: %w", err) |
There was a problem hiding this comment.
issue (llm): The error wrapping directive %w is used with fmt.Errorf for error wrapping. For logging purposes, especially with log.Warnf, you should use %v to format the error. This prevents potential confusion and maintains consistency in error logging.
* 🐛 Fixed issue with memory leak on socket-server * 🐛 Fixed issue with memory leak with websocket
1 participant
No description provided.