search

hyperium / http

Rust HTTP types
Apache License 2.0
1.15k stars 285 forks source link

Tests and fixes for remove_entry and remove_entry_mult #449

Closed dekellum closed 3 years ago

dekellum commented 3 years ago

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

dekellum commented 3 years ago

So as shown, I can fix the previously introduced test cases with 340ee18, but I'm not particularly confident about the (unsafe) links handling of the fix, nor happy about the further code duplication (in an already large HeaderMap implementation). If this is the right direction, then I should be able to collapse back to one remove_extra_value that takes Option<RawLinks<T>> to avoid the duplication.

I hope the tests are at least useful. Does this suggest any different fix?

dekellum commented 3 years ago

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.

dekellum commented 3 years ago

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 commented 3 years ago

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