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.

Sign up for GitHub

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

Jump to bottom

Review ABI vNEXT. #1

Closed
wants to merge 4 commits into from
Closed

Review ABI vNEXT. #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d7bcd21
Re-add ABI vNEXT (to be reviewed).
PiotrSikora Mar 17, 2020
fc772fa
Update ABI specification with more details.
PiotrSikora Jul 17, 2020
f1b5253
Update ABI specification to use vectors.
PiotrSikora Jul 18, 2020
cc53262
Clarify gRPC field names.
PiotrSikora Aug 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions abi-versions/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# Proxy-Wasm ABI specification

## ABI versioning

The ABI is in the form of: `<major>.<minor>.<patch>`

Breaking changes require increasing the major version.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to clarify somewhere what is meant by a breaking change. One experience we've had from xDS is that distinguishing between build and runtime breakage is important (I imagine it's both in this case). Also, bug fixes can be pretty ambiguous, e.g. a fix that introduces the "correct" behavior as per the specification, but where the existing implementation(s) all followed the "incorrect" behavior can be a challenge to handle.


Adding new functions requires increasing the minor version.

Adding new enum values requires increasing the patch version.

PiotrSikora marked this conversation as resolved.
Show resolved Hide resolved
Wasm modules should export `proxy_abi_version_<major>_<minor>_<patch>` function, indicating which
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that "Wasm modules must export..." otherwise they will not work, correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

version of the ABI they were compiled against.

Host environments should be able to support multiple versions of the ABI.
PiotrSikora marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is "should," then what should a host do if it does not? "Must" it return a readable error and refuse to start the module?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.


## Naming conventions

All functions are using `snake_case`, and they start with the `proxy_` prefix. Implementation
specific extensions should use different prefixes (e.g. `envoy_`, `nginx_`, `ats_`, etc.) in order
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markdroth will we ever have Wasm gRPC interceptors? Have you folks been tracking these ABI specs? This is another purportedly multi-client ABI/API that might benefit from gRPC input if you plan on supporting at some point in the future.

to avoid polluting the common namespace. However, this is highly discouraged, and should be used
only for implementing optional proxy-specific functionality, otherwise we’re going to lose
portability.

Function names are in the form of: `proxy_<verb>_<namespace>_<field>_<field>` (e.g.
`proxy_on_http_request_headers`, `proxy_get_map_value`, etc).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nitty, but on is a preposition or adverb?


## Memory ownership

No memory-managed data is passed to Wasm module in any of the event callbacks, and Wasm module must
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the term "memory-managed data" needs to be spelled out in a specification like this. It means different things in different situations, e.g. depending on who is managing the data, whether you have GC, etc. I think the point you want to make is that ownership of all memory that participates in ABI calls belongs to the Wasm code, it's allocated/freed by it. So, you might want to say that "No host managed memory is referencable by a Wasm module" or something like that as well. This is all semantics, but I think it makes the spec more readable.

request memory-managed data from the host environment if it’s interested in it, e.g.
`proxy_on_http_request_body` only passes the information about the number of available request body
bytes, but the module must request them using `proxy_get_buffer`. When that happens, the host
environment will request the Wasm module to allocate memory using `proxy_on_memory_allocate` and
copy requested data into it. In doing so, it passes memory ownership of that data to the Wasm
PiotrSikora marked this conversation as resolved.
Show resolved Hide resolved
module, which is expected to free this memory when it no longer needs it.

SDKs implementing this ABI are expected to automatically handle freeing of the memory-managed data
for the developers.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to say anything about the data that the Wasm module sends TO the host? For instance, when calling proxy_log, can the module assume that the host takes ownership of the data passed and that the caller is free to free it when the call returns?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the host doesn't take the ownership of the data referenced in the hostcalls.

Yes, the caller is free to free all parameters as soon as the function returns. This is the standard behavior for virtually (literally?) any programming language, so I'm not sure if that's something worth pointing out... it might result more confusion than anything else, IMHO.

Loading