Evan Thompson (Nov 01 2022 at 16:36): Evan Thompson (Nov 01 2022 at 16:36):Hey folks, when starting to use the JIT module, I noticed there are a number of places where it expects or panics on syscall errors, for example this panic on mprotect or this one during function allocation.
I’d like to update these to propagate the errors onward instead, as just crashing can be a nasty source of correlated failure in distributed systems, eg in Linux if the vm.max_map_count for the process is mistakenly configured too low, you would see correlated panics when each node tries to mmap or mprotect one too many times
Doing so would involve updating some apis like set_readable_and_executable and finalize_definitions to return Result types and let the caller handle the error (probably initially by just unwrapping, punting the issue upward a few layers)
How stable are the JIT module apis, is it generally OK to update them in this manner?
Andrew Brown (Nov 01 2022 at 18:19):I would expect this to be an OK refactoring; @bjorn3 might have more opinions.
bjorn3 (Nov 01 2022 at 18:23):Returning errors would be fine, however be sure to ensure that all invariants are upheld even if an error is returned. For example if you want to remove the expects in finalize_definitions (which I don't think you should as they are programmer errors, not environment errors) you have to put back all not yet processed functions or data objects to finalize.
Evan Thompson (Nov 01 2022 at 18:41):Thanks! I wasn't planning to remove the existing expects (for function and data object compilation) from finalize_definitions, but I was planning to add some new error handling. For example, finalize_definitions calls self.memory.code.set_readable_and_executable which is really a wrapper around syscalls like mprotect, which can fail for a variety of reasons not programmer-error-related
bjorn3 (Nov 01 2022 at 21:10):Yeah, those are totally reasonable to turn into Result returning functions.
bjorn3 (Nov 01 2022 at 21:12):You should however probably ensure that if mprotect fails that the respective memory region is not marked as having been made read+execute in the state of self.memory.code.
Last updated: Dec 23 2024 at 12:05 UTC