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

Introduce `drop` to replace `ignore (unlock ..)` for more lock-type-safety #3783

Merged
merged 2 commits into from Mar 18, 2019

Conversation

Projects
None yet
3 participants
@gasche
Copy link
Member

gasche commented Mar 14, 2019

This PR, coming out of joint work with @Armael, can be summarized by the corresponding mli documentation:

(** Releases any locks on the given global state and then ignores it.
    Using [drop gt] is equivalent to [ignore (unlock gt)],
    and safer than other uses of [ignore]
    where it is not enforced by the type-system
    that the value is unlocked before it is lost.
*)
val drop: 'a global_state -> unit

The idea is applied to all three kinds of states-with-locks (global, switch, repos), and then drop is consistently used within the whole opam codebase, to remove ignore on possibly-still-locked locks as a code smell. See the commit message of the first commit for more design details.

Example from the patch:

- ignore (OpamSwitchState.unlock st);
- ignore (clear_switch gt switch)
+ OpamSwitchState.drop st;
+ OpamGlobalState.drop (clear_switch gt switch)

I didn't find any instance where locks were silently ignored incorrectly. Besides the fact that the resulting code is generally nicer, the point of porting the whole opam codebase is to encourage clueless users of opam-lib (us, for example) to follow the same style and avoid mistakes there.

gasche added some commits Mar 14, 2019

introduce a `drop` function to unlock and ignore state
Using `ignore` when dealing with locks introduces a risk of silently
dropping a lock without unlocking it. The current opam codebase
uses the `ignore (unlock ...)` pattern which is always safe,
but also other patterns such as `ignore (pin ...)` which cannot be
checked locally, but depend on the use of `with_` above.

The goal of this `drop` function is to completely replace the usage of
`ignore` on lock types, enforcing lock-safety by design.

Note: profusely using `drop` results in `unlock` often being called
several time on the same lock (typically the lock is scoped under
a `with_` and would be dropped anyway), but this perfectly fine as
`unlock` implementations can be used multiple time on the same lock
(unlocking is idempotent).

A previous version of this API change restricted `drop` to `unlocked`
arguments (and was an alias for `ignore`), but we found that this
doesn't cover many use-cases of `ignore` on locks in the opam
codebase, necessiting an extra `with_drop` variable for total
coverage. The current, more powerful `drop` function (suggested by
Armaël Guéneau) is more flexible and can cover all `ignore` use-cases.

Note: `drop` is implemented using `let _ = unlock lock in ()` instead
of `ignore` directly, so that you can find code to improve by grepping
for `ignore` without seeting these false positives.
@AltGr

This comment has been minimized.

Copy link
Member

AltGr commented Mar 18, 2019

Indeed, this looks better, thanks!
The idea was to rely on the with_ functions as much as possible, but in practice many use-cases ended up much more complex than that :)

@AltGr AltGr merged commit ea1886b into ocaml:master Mar 18, 2019

2 checks passed

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

@rjbou rjbou added this to the 2.1.0 milestone Mar 20, 2019

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.