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

Tests and fixes for remove_entry and remove_entry_mult #449

Merged
merged 7 commits into from
Dec 11, 2020

Conversation

dekellum
Copy link
Contributor

@dekellum dekellum commented Dec 1, 2020

Initially this just adds test coverage for header::OccupiedEntry::remove_entry_mult, reproducing #446 and will fail CI.

This is just a self contained test case, currently reproducing the
panic of hyperium#446.
@dekellum

This comment has been minimized.

@dekellum
Copy link
Contributor Author

dekellum commented Dec 1, 2020

Also note that if I backport these tests to v0.1.19 (immediately before 82d53db, the seemingly most relevant change), the tests fail in a similar fashion, so this is a long standing bug.

In order to remove extra values prior to remove_found, we must collect
them eagerly. Eager collection is based on:

    commit 8ffe094
    Author:     Sean McArthur <sean@seanmonstar.com>
    AuthorDate: Mon Nov 25 17:34:30 2019 -0800
    Commit:     Sean McArthur <sean@seanmonstar.com>
    CommitDate: Tue Nov 26 10:03:09 2019 -0800

	Make ValueDrain eagerly collect its extra values

...which was reverted in 6c2b789.
@dekellum
Copy link
Contributor Author

dekellum commented Dec 2, 2020

The tests added in 5575c13 showed that tentative fix 340ee18 was insufficient, now removed from this draft.

The tests added in e419939 show the problem also extends to the OccupiedEntry::remove_entry method.

These tests are currently failing.

@dekellum
Copy link
Contributor Author

dekellum commented Dec 2, 2020

Comparing OccupiedEntry::remove_entry to HeaderMap::remove (which doesn't have the problem, also tested) gave the clue that remove_all_extra_values must be called before remove_found! The fix for remove_entry (9f6a750) was then simple enough. For remove_entry_mult I used a previously reverted commit by @seanmonstar for eager draining, and similarly re-ordering the call to remove_found (27d993c). I'm now reasonably confident of the fix.

Fixes #446

@dekellum dekellum marked this pull request as ready for review December 2, 2020 20:38
@dekellum dekellum changed the title Tests and exploratory fix of remove_entry_mult Tests and fixes for remove_entry and remove_entry_mult Dec 2, 2020
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work!

@seanmonstar seanmonstar merged commit 722df55 into hyperium:master Dec 11, 2020
@dekellum dekellum mentioned this pull request Dec 14, 2020
BenxiangGe pushed a commit to BenxiangGe/http that referenced this pull request Jul 26, 2021
* add test case for OccupiedEntry::remove_entry_mult

This is just a self contained test case, currently reproducing the
panic of hyperium#446.

* expand test cases for remove_entry_mult

* add multiple remove_entry_mult call tests to show more issues

* test repeated HeaderMap::remove for comparison

* tests showing similar problem with remove_entry on extra values

* fix remove_entry by moving remove_found after remove_all_extra_values

* fix remove_entry_mult by eager collection of extras before remove_found

In order to remove extra values prior to remove_found, we must collect
them eagerly. Eager collection is based on:

    commit 8ffe094
    Author:     Sean McArthur <sean@seanmonstar.com>
    AuthorDate: Mon Nov 25 17:34:30 2019 -0800
    Commit:     Sean McArthur <sean@seanmonstar.com>
    CommitDate: Tue Nov 26 10:03:09 2019 -0800

	Make ValueDrain eagerly collect its extra values

...which was reverted in 6c2b789.

Closes hyperium#446
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