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

Make hijackConn behaviour compliant with net.Conn interface #1629

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

askolesov
Copy link
Contributor

@askolesov askolesov commented Oct 4, 2023

Currently, the way fasthttp.hijackConn behaves is different from what can be expected with the net.Conn interface.

Normally, when you close a net.Conn, if you try to read or write again, you get errors.

But with fasthttp.hijackConn, when you close it, it gets cleaned and put back in the pool. If you try to read from or write to it again, it can cause two problems:

  1. If it hasn't been reused yet, it might cause null reference exception because the underlying connection is nil.
  2. If it has been reused, you might end up reading from or writing to a completely different connection, which is very bad.

I suggest not reusing hijackConn when KeepHijackedConns is enabled (it still will be reused when this option is disabled). This might make things a bit slower, but it will make the behavior more predictable for users.

We've had issues with this in our project, and it seems like the easiest way to fix them is with this pull request.

@askolesov
Copy link
Contributor Author

askolesov commented Oct 5, 2023

There was a typo led to stack overflow, fixed that.

@askolesov
Copy link
Contributor Author

Could you please run tests again? The issue doesn't seem to be related to the PR.

@erikdubbelboer erikdubbelboer merged commit c6a17b9 into valyala:master Oct 6, 2023
12 of 14 checks passed
@erikdubbelboer
Copy link
Collaborator

I agree, seems like a good change. Thanks!

@askolesov
Copy link
Contributor Author

Thank you!

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.

2 participants