-
Notifications
You must be signed in to change notification settings - Fork 154
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
irmin-pack: treat unhandled exception in async task as a failure #2163
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2163 +/- ##
==========================================
+ Coverage 68.06% 68.08% +0.01%
==========================================
Files 134 134
Lines 16160 16161 +1
==========================================
+ Hits 11000 11003 +3
+ Misses 5160 5158 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
It looks very reasonable 👍 |
113046e
to
a72f2c8
Compare
This fixes a bug where unhandled exceptions for an async task were treated as a `Succes instead of `Failure.
a72f2c8
to
a87d392
Compare
@art-w thanks for taking a look. I'll leave the functionality as-is for now. I have adjusted the comment. |
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.
LGTM, thanks!
…min-pack, irmin-pack-tools, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.7.0) CHANGES: ### Added - **irmin** - Add `Conf.pp` and `Conf.equal` to print and compare configuration values (mirage/irmin#2227, @samoht) - Add a `clear` optional arguments to all function that adds a new commit: `Commit.v`, `set`, `set_tree`, `remove`, `test_and_set`, `test_and_set_tree`, `test_set_and_get`, `test_set_and_get_tree`, `merge`, `merge_tree` and `with_tree`. This new argument allows to control whether the tree caches are cleared up after objects are exported to disk during the commit. (mirage/irmin#2225, @samoht) - **irmin-pack** - Add configuration option, `lower_root`, to specify a path for archiving data during a GC. (mirage/irmin#2177, @metanivek) - Add `is_split_allowed` to check if a store allows split. (mirage/irmin#2175, @metanivek) - Add `add_volume` to allow creating new empty volume in lower layer. (mirage/irmin#2188, @metanivek) - Add a `behaviour` function to the GC to check wether the GC will archive or delete data. (mirage/irmin#2190, @Firobe) - Add a migration on `open_rw` to move the data to the `lower_root` if the configuration was enabled (mirage/irmin#2205, @art-w) ### Changed - **irmin** - Expose type equality for `Schema.Info` to avoid defining the `info` function multiple times when using similar stores (mirage/irmin#2189, mirage/irmin#2193, @samoht) - **irmin-pack** - GC now changes its behaviour depending on the presence of a lower layer. (mirage/irmin#2190, @Firobe) - Split now raises an exception if it is not allowed. It is not allowed on stores that do not allow GC. (mirage/irmin#2175, @metanivek) - GC now supports stores imported V1/V2 stores, in presence of a lower layer only. (mirage/irmin#2190, @art-w, @Firobe) - Upgrade on-disk format to version 5. (mirage/irmin#2184, @metanivek) - Archive to lower volume does not copy orphaned commits. (mirage/irmin#2215, @art-w) ### Fixed - **irmin-pack** - Unhandled exceptions in GC worker process are now reported as a failure (mirage/irmin#2163, @metanivek) - Fix the silent mode for the integrity checks. (mirage/irmin#2179, @icristescu) - Fix file descriptor leak caused by `mmap`. (mirage/irmin#2232, @art-w)
This fixes a bug where unhandled exceptions for an async task were treated as a
Succes
instead ofFailure
. I discovered this while looking into #2161.This PR addresses this issue by:
Previously, we used
SIGKILL
to preventat_exit
hooks from executing. I preserved this functionality butleft a comment in the code sinceI'm not sure if we need or want to do this or not. The benefit I see is that you don't accidentally run exit hooks from the parent process, which seems beneficial when running within applications that we do not control (like octez). A drawback is that maybe the logs aren't flushed, which would mean things like the error about the unhandled exception will not appear in logs, but I don't think this is something we can directly control outside of justStdlib.flush_all ()
and a prayer 👼. I welcome thoughts and opinions on this!