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

[BUG] Unnecessary socket re-creation inside with statement in whisper_online_server.py #90

Closed
DoiiarX opened this issue May 20, 2024 · 3 comments

Comments

@DoiiarX
Copy link
Contributor

DoiiarX commented May 20, 2024

Is this re-creation of the socket intended for a specific reason, or is it an oversight that needs fixing? I would like to contribute by addressing this issue if it is indeed unintended.

Description

In the whisper_online_server.py file, there is an unnecessary re-creation of a socket object inside a with statement which can potentially lead to resource leakage. The existing code snippet is:

with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
    s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)

Issue

The socket s is being recreated inside the with statement, which overrides the original socket created by the with statement itself. This not only defeats the purpose of the with statement which is supposed to handle the creation and destruction of the socket but also potentially leaks the original socket since it's not properly closed.

Expected Behavior

The socket should be created once and managed by the with statement to ensure proper resource management without leaks. The code should simply be:

with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
    # Use socket `s` here

Steps to Reproduce

This is a static code issue and can be observed directly by reviewing the code in whisper_online_server.py.

Possible Solution

Remove the line that unnecessarily recreates the socket inside the with statement.

Thank you for addressing this issue.

@Gldkslfmsd
Copy link
Collaborator

Gldkslfmsd commented May 20, 2024

Thanks! It's an oversight.
Have you tested your PR? If yes and it works, I'll merge it.

@DoiiarX
Copy link
Contributor Author

DoiiarX commented May 21, 2024

Thanks! It's an oversight. Have you tested your PR? If yes and it works, I'll merge it.

yes. no problem.

@Gldkslfmsd
Copy link
Collaborator

thanks!

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

No branches or pull requests

2 participants