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

Fix Stream.junk on channel streams #1714

Merged
merged 3 commits into from Apr 11, 2018

Conversation

Projects
None yet
2 participants
@nojb
Copy link
Contributor

nojb commented Apr 10, 2018

See MPR#7769.

A bug happens when doing Stream.junk on a channel stream when the buffer is empty: Stream.junk never refills the buffer and simply increases the (empty) buffer index b.ind. The next time Stream.next is called, the buffer is refilled and the index is reset to 0, forgetting that Stream.junk was called.

Rather, we should refill the buffer if needed in Stream.junk.

@gasche

gasche approved these changes Apr 10, 2018

Copy link
Member

gasche left a comment

I believe that the fix is correct, modulo a minor comment.

(I'm positively surprised by how much cleaner the Stream codebase is than the last time I looked at it. Thanks @chambart!)

(We could now use inline records to optimize the 'a t representation, but I don't know if we still have performance-needy Stream users.)

| Sbuffio b -> s.count <- (succ s.count); b.ind <- succ b.ind
| Sbuffio b ->
if b.ind >= b.len then fill_buff b;
if b.len == 0 then s.data <- Sempty

This comment has been minimized.

@gasche

gasche Apr 10, 2018

Member

I suppose this is a typo for b.len = 0.

This comment has been minimized.

@nojb

nojb Apr 10, 2018

Author Contributor

Not really -- I used the style present in the rest of the file.

This comment has been minimized.

@gasche

gasche Apr 10, 2018

Member

Oh, sorry.

@nojb nojb force-pushed the nojb:fix_stream_bug branch from 914cbae to 34c88a1 Apr 10, 2018

@nojb nojb merged commit cc83b36 into ocaml:trunk Apr 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 11, 2018

@nojb feel free to cherry-pick in 4.07.

(Cherry-picking a multi-commit change is kind of tricky, good luck with git. I can do it myself if you wish. My approach is to create a copy of the branch with -4.07 in the name, then do a git rebase --onto 4.07 trunk <branchname>, then merge that into 4.07.)

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Apr 11, 2018

I will give it a try, thanks!

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Apr 11, 2018

This is 20a87a8...6245f19 on 4.07.

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Apr 11, 2018

By the way, if your version of git is not too old, it is not hard to cherry-pick multiple commits. This is what I did to cherry pick this PR into 4.07:

$ git checkout 4.07
$ git cherry-pick trunk^1..fix_stream_bug

where trunk^1 is the commit on trunk just "before" the merge of the branch fix_stream_bug (if you do not put in the ^1, the commit range is empty due to fix_stream_bug having already been merged into trunk).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.