Skip to content
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

chore(rust): change ockam_core error domain to non-static string #967

Closed
wants to merge 1 commit into from

Conversation

spacekookie
Copy link
Contributor

When sending Error types over the network, 'static lifetime strings can't be deserialised. This change is required for the current ockam worker supervisor design.

Change impact

This change has some obvious implications for ockam_ffi, and maybe others.

In ockam_ffi the error representation can still hold a *const c_char, but it needs to be contstructed differently. This is done via std::ffi::Cstring, which allocates the string in a way that C will understand. Then calling into_raw() returns a pointer to the location.

This will leak memory if the C-calling code doesn't free() the string!

In no_std settings we will need to find a way to do the equivalent thing without the standard allocator.

Alternatives

I opted to use the ockam_node::Error type for the supervisor system because it is immediately available to users, and adds a good mechanism for namespaced error codes. There are however other approaches that we can take with this design:

Separate, concrete error type

There can be a special type exposed by ockam_core which allows users to fill in their own error data without having to move this requirement to the ockam_core::Error.

The main downside to this approach is code duplication. Based on the requirements, this type may end up being a near-identical copy of the ockam_core::Error type.

Associative type

There can be another associated type on the Worker trait, meaning that users can make the supervisor error type custom to their workers.

Cons:

  • Requires another type Foo = ... line in the worker definition, which increases general boilerplate, even if they don't use the supervisor feature
  • All workers in a system may have to share the same type anyway, because otherwise incompatible error types may be dropped by the supervisors

For no_std settings we will have to find a way to do this without relying on the libstd CString type. But I the heapless crate we're using probably has some way of handling this too.


Anyway, this change doesn't urgently need to land, but it may be worth some thoughts with regards to how to achieve the most flexible design in this domain.

When sending Error types over the network, static-lifetime strings
can't be deserialised.  This change has some implications for
ockam_ffi, which now needs to allocate some memory for the C string.
@jdspdx
Copy link
Contributor

jdspdx commented Feb 23, 2021

Thanks for this awesome detailed write up, @spacekookie. @SanjoDeundiak when you get a chance, could you spend some time looking over this and thinking about the possible impacts?

@SanjoDeundiak
Copy link
Member

@spacekookie @jared-s @mrinalwadhwa @mikelodder7
Static domains simplified things a lot, they don't require allocations and you can safely send them over threads or ffi bound without worrying. From the other hand deserializing such struct would require knowing all possible error domains, which is what we were avoiding while designing errors.
I'm not aware of what is the need in sending those errors over the network (we hadn't thought about this use case). Is there a way to use different type of error for sending over the network, will it require a lot of changes?

@SanjoDeundiak
Copy link
Member

Let's say we have Error1 with static domains and Error2 with dynamically allocated domains. Obviously Error1->Error2 conversion is simple, Error2->Error1 is impossible (well, possible, but horrible). Do these constraints good enough to build what you're trying to build @spacekookie ?

@spacekookie
Copy link
Contributor Author

@SanjoDeundiak

Static domains simplified things a lot, they don't require allocations and you can safely send them over threads or ffi bound without worrying.

Replacing &'static str with String would not impact the fact that Error is still Send. Currently Error doesn't derive Copy either, which is the main benefit/ possible optimisation of having a non-heap allocated string type in the error.

From the other hand deserializing such struct would require knowing all possible error domains, which is what we were avoiding while designing errors.

I'm not aware of what is the need in sending those errors over the network (we hadn't thought about this use case). Is there a way to use different type of error for sending over the network, will it require a lot of changes?

Considering that ockam is a highly distributed system, and errors may occur on nodes that are not the primary caller of a function chain, I feel like it is fundamental to be able to send errors that occur in operations across the network.

I specifically ran into this issue because I wanted to use the Error type already present in ockam for the supervisor system, but I could see this being a problem in other scenarios in the future as well.

Let's say we have Error1 with static domains and Error2 with dynamically allocated domains. Obviously Error1->Error2 conversion is simple, Error2->Error1 is impossible (well, possible, but horrible). Do these constraints good enough to build what you're trying to build @spacekookie ?

If we end up keeping the error domain as a static string, I would probably not interact with the existing error type and have a dedicated type for supervisor errors that users can encode their state into.

However, I feel like this would end up being confusing to users, and severely limit the use-case of the actual error system (user code will fail if they try to send an error that the context API gives them for example).

@spacekookie
Copy link
Contributor Author

Merged as #1381

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants