small cleanups #2062
small cleanups #2062
Conversation
src/irmin-pack/unix/ext.ml
Outdated
| @@ -593,7 +593,8 @@ module Maker (Config : Conf.S) = struct | |||
| implementation detail. This is safe since the callback | |||
| [f] is attached to [t.running_gc.promise], which is | |||
| referenced for the lifetime of a GC process. *) | |||
| let _ = Lwt.bind x.promise f in | |||
| let p = Lwt.bind x.promise f in | |||
| Lwt.on_success p Fun.id; | |||
There was a problem hiding this comment.
@metanivek what do you think about this change? or we could instead use on_failure to log a message.
There was a problem hiding this comment.
I think it is unnecessary -- what is this line gaining for us? We could log a message on failure, but I don't see a reason to do so at this call site.
There was a problem hiding this comment.
you're probably right its unnecessary, I removed it
There was a problem hiding this comment.
👍 To explain my reasoning (which is open to critique if it is not accurate!): x.promise (and x.resolver) exist just so that users can run code when a GC finalizes. The call to Lwt.bind attaches f to x.promise which means f will execute when we call Lwt.wakeup_later x.resolver. Lwt.on_success p Fun.id just adds Fun.id to create this promise chain: x.promise -> f -> Fun.id.
There was a problem hiding this comment.
Thanks for the explanation, makes sense. It was a "tentative" suggestion, I agree that adding a Fun.id is useless. I'm just not used on not waiting on lwt promises, in case something goes wrong. But in this case, f can add logs messages if it needs to.
There was a problem hiding this comment.
Yeah, I'll admit that it's a bit of an atypical usage of Lwt in that it is only using the promise part. Logging (and/or event reporting) is exactly how this callback is used in the Tezos integration.
Codecov Report
@@ Coverage Diff @@
## main #2062 +/- ##
=======================================
Coverage 64.34% 64.35%
=======================================
Files 131 131
Lines 15584 15577 -7
=======================================
- Hits 10028 10024 -4
+ Misses 5556 5553 -3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
icristescu
force-pushed
the
small_clean
branch
from
5bcec24
to
0ad3f90
Compare
IIRC, we used to reload the mapping file in the dispatcher, and for that we needed a
register_mapping_consumers. Now the dispatcher gets the updated mapping from the file manager at every read, so there is no need formapping_consumers.Part of #2039.