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

Public C API #558

Open
32 of 49 tasks
gumb0 opened this issue Sep 29, 2020 · 10 comments
Open
32 of 49 tasks

Public C API #558

gumb0 opened this issue Sep 29, 2020 · 10 comments

Comments

@gumb0
Copy link
Collaborator

gumb0 commented Sep 29, 2020

@Qix-
Copy link
Contributor

Qix- commented Mar 12, 2021

Can a roadmap item be added for all-around better error handling? The existing API has zero error handling, and though I'm moving to Fizzy from wasm3 due to some convoluted blocking bugs, wasm3 has much better error handling in its API that I'm really missing.

Right now, it's just "welp, it failed" if some of the resulting pointers are NULL - there's nothing to help understand why something failed.

@axic
Copy link
Member

axic commented Mar 12, 2021

You are absolutely right. I was integrating fizzy into something else the other day and the lack of error message propagation is annoying. We'll think about a good API for this for the C layer, but if you have some ideas please do not be afraid to share them.

@Qix-
Copy link
Contributor

Qix- commented Mar 12, 2021

@axic an extra parameter that accepts a pointer to an error code would be great.

Alternatively, changing the return types to be an error code (or success code of course) and having the pointer results be returned via pointer-to-pointer arguments.

For example:

FizzyModule *fz_mod;

int r = fizzy_parse(
	&fz_mod,
	mod_bytes.data(),
	mod_bytes.size()
);

if (r != 0) {
    fprintf(stderr, "error: failed to parse module: %s\n", fizzy_strerr(r)); 
} else {
    assert(fz_mod != NULL);
    /* fz_mod is valid */
}

@axic
Copy link
Member

axic commented Mar 17, 2021

I think we want to go beyond error codes to have the capability of returning messages. While error codes work well for many cases, they do not for telling about import resolutions problems (i.e. including namespace, module, etc.)

There are two problems with returning messages though: a) who manages the memory allocated for them; b) can we avoid allocating memory.

In the following option FizzyError is just a nice container for a fixed-size buffer. In this case the error message buffer lives on the stack of the caller:

FizzyError err;
fizzy_parse(..., &err);
... = fizzy_error_msg(&err); // returns const char *

In the following option we use a global buffer, which can be queried. The downside is that subsequent calls overwrite it and it is not thread safe.

fizzy_parse(...);
... = fizzy_error(); // returns const char*

Lastly, we return a message object which has to be explicitly free'd by the caller:

FizzyError* err;
fizzy_parse(..., &err);
... = fizzy_error_msg(err); // returns const char *
fizzy_error_free(err);

I'm sure @chfast and @gumb0 have some better ideas.

@axic
Copy link
Member

axic commented Mar 17, 2021

@Qix- another question: should we have a public C++ API, would that be preferable? I'm not saying we'll have it anytime soon, but it is only missing due to lack of time.

@chfast
Copy link
Collaborator

chfast commented Mar 17, 2021

Something like this seems ok (just a sketch), but happy to review C API design from other libraries.

struct FizzyError { int error_code; const char* message; }

FizzyError err;
fizzy_parse(..., &err);
if (err.error_code != 0)
    printf("%s", err.message) && exit(err.error_code);
fizzy_instantiate(..., &err);
if (err.error_code != 0)
    printf("%s", err.message) && exit(err.error_code);
fizzy_free_error(&err);

So the FizzyError is reusable and optional (you can pass NULL meaning I don't care about error.

@axic
Copy link
Member

axic commented Mar 17, 2021

Yes, that matches my first idea. Though I was wondering if the struct should be opaque or not.

However, when it is reusable then subsequent users must check if err.message is not null and free it.

@chfast
Copy link
Collaborator

chfast commented Mar 17, 2021

However, when it is reusable then subsequent users must check if err.message is not null and free it.

No. This can be handled inside Fizzy. Only after the last use the manual free is needed.

Alternatively, we can adopt this Store think present in many wasm implementation, including wasm-c-api.

@axic
Copy link
Member

axic commented Mar 17, 2021

However, when it is reusable then subsequent users must check if err.message is not null and free it.

No. This can be handled inside Fizzy. Only after the last use the manual free is needed.

My wording was unclear, by "subsequent users" I meant subsequent calls. So I agree 😉

@Qix-
Copy link
Contributor

Qix- commented Mar 18, 2021

The problem with error strings is two-fold:

  • They are not (easily) localizable.
  • They are not guaranteed to be programmatically switchable (e.g. if (errcode == FIZZY_ENOSUCHIMPORT) { /* ignore */ }) even if you use extern strings.

This is why many C libraries provide a const char * strerror(int) function that translates error codes to statically allocated messages (e.g. POSIX C, libuv, etc.) allowing for other consumers of the library to localize if need-be.

I would recommend doing it this way.


In the event you do decide to go with strings, please under no circumstances perform an allocation. This could cause a cascade of failures that makes debugging (especially in production) very difficult (a memory error would cause a memory-related crash, masking the originating Fizzy-related error - or, many error codes could be generated that are ultimately ignored, but a forgotten free() causes a memory leak over time, etc.)

Any error-related strings should be strictly statically allocated.

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

No branches or pull requests

4 participants