Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upError handling fixes #134
Merged
Error handling fixes #134
Conversation
|
Looks great, some comments:
Thanks for doing this! :) |
I believe the whole premise of this error was invalid in the first place: as far as I can see, there is actually nothing wrong with using IPC channels during serialisation or deserialisation of an IPC channel message -- in fact we added a test case for this in ca63fdb . So what this handling really did (originally), was complaining -- and hiding the actual error message -- when there was an I/O error while doing recursive IPC during deserialisation; whereas otherwise recursive I/O worked just fine. (Also, the `send()` part of the implementation was never correct: it translated errors in the low-level send operation, where no recursion can actually happen; while errors during serialisation just panic...) Note that 1f1d092 indeed changed the error handling to pass through the real I/O errors during deserialisation, as it should. The `recursive_io_error()` would instead now be triggered on a condition that can never actually come up, as far as I can see... Summing up, neither were we actually detecting recursive I/O, nor should we. As a nice side effect (in fact what made me look into this in the first place), with this bogus handling gone, we now no longer use the never stabilised (and now deprecated) `RefCell::borrow_state()`, and thus can drop the local `RefCell` fork.
Just like deserialisation in `recv()`, serialisation during `send()` can trigger I/O (or possibly other?) errors. Pass these on to the caller, rather than throwing a `panic!`.
I'm pretty sure we should restore the thread `OS_IPC_CHANNELS_FOR_DESERIALIZATION` and `OS_IPC_SHARED_MEMORY_REGIONS_FOR_DESERIALIZATION` values to their previous states also when deserialisation fails, not only when it succeeds.
antrik
force-pushed the
antrik:fix-recursive-io-error-handling
branch
from
67a78f8
to
a5b568b
|
@emilio added a comment regarding the error handling. I don't think there is any policy about only using features that have been stable for a long time. (This would actually prevent fixing warnings about no longer needed feature gates?) I believe we already use the syntax in various Servo repositories; and I'd be surprised if it's not already being used in various external dependencies as well... Not changing the spelling of "serialisation", since it's not actually a mistake, as I mentioned on IRC :-) |
|
@bors-servo r=emilio |
|
|
bors-servo
added a commit
that referenced
this pull request
Jan 30, 2017
Error handling fixes Some fixes and cleanups for "recursive I/O" and related error handling.
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
antrik commentedJan 17, 2017
Some fixes and cleanups for "recursive I/O" and related error handling.