-
Notifications
You must be signed in to change notification settings - Fork 283
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
IH-633: Transition away from exception-raising Hashtbl.find and Unix.getenv #5751
IH-633: Transition away from exception-raising Hashtbl.find and Unix.getenv #5751
Conversation
e1759ac
to
a842901
Compare
| None -> | ||
raise | ||
(Api_errors.Server_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define internal_error fmt
in helpers and use that.
let fail fmt =
Printf.kprintf
(fun msg -> raise Api_errors.(Server_error (import_error_generic, [msg])))
fmt
The above code is not raising internal_error
but demonstrates how to define a function that raises an exception and takes printf-style arguments. You can use that to simplify other places that raise internal errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there is a similar function defined in xenops_server.ml
, for example:
let internal_error fmt =
Printf.kprintf
(fun str ->
error "%s" str ;
raise (Xenopsd_error (Internal_error str))
)
fmt
Will open a separate ticket for this --- there are quite a few such cases with internal Printf.sprintf
Due to the size of this PR, I will be pushing fixup commits to ease the review process. Will squash these at the end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Pedantically, there is a case where you can η-reduce - nevermind, we shouldn't be making unrelated changes here (I guess do them if you want so long as they are obviously correct and won't impair review?).(fun s -> Stunnel.disconnect s)
.
Also, if you want to be consistent with using Option.map
and friends to outline casing behaviour, there are a few cases where fold
could be used, e.g.:
let lookup table r =
match Hashtbl.find_opt table r with
| Some x ->
Ref.of_string x
| None ->
Ref.null
could become
let lookup table r =
Hashtbl.find_opt table r
|> Option.fold ~none:Ref.null ~some:Ref.of_string
The same change is applicable to attempt_restart
, to_wait
, etc. This is largely down to individual preference though, I think the explicit match
is completely acceptable.
I appreciate these changes. I have no doubt that you've probably mentally recorded other changes that could be made within the vicinity of these changes (but correctly didn't do them here as they're outwith the scope of this changeset): I suggest we look into documenting the potential further changes (of similar vein to these changes) explicitly somewhere; as there's many good stdlib API improvements - of a similar scope to these changes - available to us once we manage to bump the OCaml version up. There was a suggestion to make the quality gate a bit more flexible using semantic grepping, that could be an avenue to categorise all of these improvements (beyond that which is given to us by quality gate which can't capture every case).
Nice catch! I've been trying to keep this PR to changes that do not change the logic of code -- this one does not, however, a simple grep indicates there are at least 50 such cases in the codebase!
Might just as well put this in a separate ticket/PR - though there is little practical benefit, it gets boiled down to the same code
I myself find that the parseability of |
Indeed, it's only a stylistic concern; this case is a minor one but there are areas of the codebase where eta-reduction could be applied several times, which cleans things up a lot, visually (typical examples would be with |
0548ee7
to
3ac45d4
Compare
Had to rebase on top of master to get the Clock module, so hashes have changed. Otherwise review fixes are still in separate fixup commits. |
3ac45d4
to
88ba347
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Has your comment being addressed, @psafont? Please revisit this such that we can merge this. |
@@ -126,9 +126,13 @@ let canonicalise x = | |||
if not (Filename.is_relative x) then | |||
x | |||
else (* Search the PATH and XCP_PATH for the executable *) | |||
let paths = Re_str.split colon (Sys.getenv "PATH") in | |||
let paths = | |||
Re_str.split colon (Option.value (Sys.getenv_opt "PATH") ~default:"") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for next time: Re is not thread-safe, replace it if it's easy to do so
Please squash the commits |
dd27180
to
b2eeff2
Compare
Squashed the fixes. Commits are still split into several as was suggested by Christian initially, so that we could revert only one if needed later (especially in the case of the more complex refactored files) |
explicit handling of failure cases. OCaml's stdlib has Sys.getenv_opt since 4.05. Some of the newer code already uses it, and some of the old code handled exceptions (so could nicely be transitioned to handling options instead). Some, however, did not handle failure at all. This commit remedies that. In most cases, getenv is used to query the PATH variable (before adding another directory to it, for example), in which case there is a nice default value of "". In some cases, the environment variable is required to be present to proceed, then there is a failure of some kind raised with the appropriate message. A test case was added to the quality-gate.sh script to prevent introduction of the exception-raising Unix.getenv into new code. Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
This avoids two traversals in the cases where Hashtbl.mem is used right before Hashtbl.find: avoiding two traversals, possible data races and the possibility where one would be changed without the other, introducing bugs. Additionally, it handles failure explicitly where it wasn't handled before, and moves from exception handling to matching on options resulting in intentions becoming clearer. This commit only changes trivial cases where little refactoring was necessary. Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
b2eeff2
to
d4be15e
Compare
Signed-off-by: Andriy Sultanov <53952748+last-genius@users.noreply.github.com>
This PR starts the transition away from exception-raising OCaml's stdlib functions
Unix.getenv
andHashtbl.find
in favour ofSys.getenv_opt
andHashtbl.find_opt
respectively.This is beneficial because:
.find
functions where gated by.mem
functions to avoid raising exceptions, instead doing just one.A naive benchmark shows the benefit:
.mem
) and getting its value (.find
).mem
would be changed but its corresponding.find
would not be.===========
This PR is structured in the following way:
Unix.getenv -> Sys.getenv_opt
change along with the quality gate test for it. Unix.getenv usage has been completely eliminated.Hashtbl.find -> find_opt
Hashtbl.find -> find_opt
, in one case I've eliminated unnecessaryHashtbl
operations altogether.Hashtbl.find
- there are 36 usages left that do not handle exceptions currently and would need to introduce it on the appropriate level.Even though the PR is relatively large, most changes are trivial and fall into several classes:
Option.value
_opt
by doingtry Some (x.find) with _ -> None
=> justx.find_opt
.find
gated by.mem
=> transition to match statement /Option.iter
/Option.map
depending on the case===========
These changes passed the BST+BVT test suites.