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

Prevent no-longer-"useful" buffers from staying in the BufferPool. #407

Merged
merged 2 commits into from Jul 29, 2015

Conversation

Projects
None yet
4 participants
@awpr
Member

awpr commented Jul 28, 2015

Previously, when 'receive' pulled a buffer and filled enough space in it to make it cease to be "useful" (i.e. have less remaining space than a certain threshold), it would be left in the BufferPool IORef and overwritten by the next 'receive' call. This broke referential transparency of the ByteStrings and caused corruption of request bodies and, less frequently, protocol synchronization data.

Fixes #406.

@creichert

This comment has been minimized.

Member

creichert commented Jul 28, 2015

I haven't had time to test this personally but it does sound like there might be an issue. Is there some way to quantify the steps you described in the issue through a test?

@awpr

This comment has been minimized.

Member

awpr commented Jul 28, 2015

I think I can reproduce the issue more reliably by testing BufferPool
itself rather than the HTTP server as a whole. For example, I think this
should reproduce the underlying issue:

  • Create a BufferPool with a 2049-byte buffer.
  • Use 'withBufferPool' to "read" 2 bytes to bring it below the "useful"
    threshold, and keep the resulting 2-byte ByteString.
  • Read some number of bytes again and check whether the old ByteString has
    the wrong contents.

I'll look into it this afternoon.

On Tue, Jul 28, 2015 at 8:54 AM Christopher Reichert <
notifications@github.com> wrote:

I haven't had time to test this personally but it does sound like there
might be an issue. Is there some way to quantify the steps you described in
the issue through a test?


Reply to this email directly or view it on GitHub
#407 (comment).

@creichert creichert added the http2 label Jul 28, 2015

awpr added some commits Jul 28, 2015

Add BufferPool test to provoke buffer clobber bug
I've confirmed this test fails when the change of
5ea6ada is reverted, and passes
otherwise.

kazu-yamamoto added a commit that referenced this pull request Jul 29, 2015

Merge pull request #407 from awpr/clear_buffer_pool
Prevent no-longer-"useful" buffers from staying in the BufferPool.

@kazu-yamamoto kazu-yamamoto merged commit 66d99c4 into yesodweb:master Jul 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 29, 2015

This is a terrible bug. I should have be more careful when merging the buffer pool patches.

@awpr's patch looks good to me, so merged.

@codedmart

This comment has been minimized.

codedmart commented Jul 29, 2015

@awpr Thanks this fixes our issue.

@kazu-yamamoto @snoyberg Any timeframe on a new release with this fix? Thanks!

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Jul 30, 2015

Need to fix #411.

@awpr awpr deleted the awpr:clear_buffer_pool branch Jul 30, 2015

@codedmart

This comment has been minimized.

codedmart commented Jul 30, 2015

@kazu-yamamoto Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment