Do not panic in Drop#13
Open
sbans-ff wants to merge 2 commits into
Open
Conversation
A fresh dependency resolution of copc-rs 0.5.0 selects las 0.9.11, which uses
laz 0.12 internally. copc-rs pins laz 0.9, so `header.laz_vlr()` returns a
laz-0.12 `LazVlr` while the compressor expects a laz-0.9 `LazVlr`:
error[E0308] --> src/writer.rs:359:52
expected `LazVlr`, found `laz::laszip::vlr::LazVlr`
So the crate does not compile for downstream consumers (which ignore the
committed lockfile) or after `cargo update`. Bump laz to 0.12 to unify on a
single laz shared with las; every laz API copc-rs uses is unchanged across
0.9 -> 0.12 (LazVlrBuilder, LazItemRecordBuilder, LazItemType, LazVlr consts,
record compressor/decompressor traits, ChunkTable).
Adds tests/roundtrip.rs (the crate had no tests): write -> read-back through
copc-rs's own writer/reader. It fails to *build* before the bump and passes
after, guarding against the dependency regressing again.
CopcWriter::drop called self.close().expect(...). is_closed is only set on a successful close(), so two reachable paths panic on drop: * a writer constructed but never written (close() -> Err(EmptyCopcFile)), and * a write that errored before close() completed. A panic in Drop while another panic is unwinding aborts the process. Make Drop a best-effort close that ignores the error; callers needing to observe close errors should finish the write explicitly. Adds tests/drop_does_not_panic.rs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CopcWriter::dropcallsself.close().expect(...).is_closedis only set on a successfulclose(), so two reachable paths panic on drop:close()returnsErr(EmptyCopcFile);close()completed.A panic in
Dropwhile another panic is unwinding aborts the process, so this is a latent crash. This makesDropa best-effort close that ignores the error; callers that need to observe close errors should finish the write explicitly rather than rely onDrop. Addstests/drop_does_not_panic.rs(panics before the fix, clean after).Stacked on #12 — the crate doesn't build against current
laswithout that fix, so this branch includes its one commit. Happy to rebase to a single commit once #12 merges.