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

Return write buffer to pool on write error #427

Merged
merged 1 commit into from Sep 24, 2018
Merged

Return write buffer to pool on write error #427

merged 1 commit into from Sep 24, 2018

Conversation

ghost
Copy link

@ghost ghost commented Sep 13, 2018

Fix bug where connection did not return the write buffer to the pool
after a write error. Add test for the same.

Bonus improvement: Adjust message and buffer size in TestWriteBufferPool
to test that pool works with fragmented messages.

@kisielk
Copy link
Contributor

kisielk commented Sep 13, 2018

Would it make more sense to put this cleanup code in a defer?

@ghost
Copy link
Author

ghost commented Sep 13, 2018

@kisielk After looking into your suggestion, I realized that I missed another error case. The code belongs in messageWriter.fatal. That method should be renamed to finishWrite for symmetry with the existing prepWrite method. I'll push a change later today.

@kisielk
Copy link
Contributor

kisielk commented Sep 13, 2018

Another thing is, should c.writer be set to nil on error as well, like it is on final, or does that not matter?

Fix bug where connection did not return the write buffer to the pool
after a write error. Add test for the same.

Rename messsageWriter.fatal method to endMessage and consolidate all
message cleanup code there. This ensures that the buffer is returned to
pool on all code paths.

Rename Conn.prepMessage to beginMessage for symmetry with endMessage.
Move some duplicated code at calls to prepMessage to beginMessage.

Bonus improvement: Adjust message and buffer size in TestWriteBufferPool
to test that pool works with fragmented messages.
@ghost
Copy link
Author

ghost commented Sep 14, 2018

Please look again.

The messageWriter.fatal method contained the code to cleanup after a message. I moved the buffer cleanup to that method and renamed the method to endMessage to make the intent of the method clearer.

I also renamed Conn.prepMessage to beginMessage for symmetry with endMessage. I also moved some duplicated code at the beginMessage call sites to beginMessage.

@ghost
Copy link
Author

ghost commented Sep 24, 2018

Can somebody please take a look?

@garyburd garyburd merged commit 3130e8d into gorilla:master Sep 24, 2018
@gorilla gorilla locked and limited conversation to collaborators Apr 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants