ynx0 / airlock Goto Github PK

Currently, methods such as Urbit#uid and Urbit#hexString are directly adapted from the typescript version of airlock, but due to differences in the languages, may yield different results. To address the ambiguity, we need to add unit tests that check for the soundness of these methods and related.

The OkHttp Response object is partially mutable. Specifically, the ResponseBody object is backed by a "one-shot" buffer that is responsible for storing the body content.

This is slightly problematic because:

1. it makes for bad ergonomics. Although not the end of the world, I cannot, for example, print the response body within the sendMessage method and still have it available for the caller, because it is consumed.
2. this behaviour is only present in the object returned by response.body(), but all other properties of the Response object are mutable. This mixed (im)mutability makes it hard to reason for an outsider about the mutability, making it error prone. in practice, this doesn't really matter cause I don't expect anyone else to really touch that area in the near future.
3. if I return the Response object untouched, it leaves a potential memory leak because the responsibility is now on the caller to call .body() or .close() on the Response/ResponseBody object, which is probably not obvious to them.

One solution that came up in a discussion $todo:link$, was to create your own InMemoryResponseBody which does a zero-copy clone of the buffer and keeps it permanently in memory.
This solution, while doable, is unwieldy because it requires not only creating this custom type, but also creating a custom Response object (e.g. InMemoryResponse) that wraps around/contains this type instead of the regular ResponseBody type. If for some reason the new class cannot simply extend Response this also hinders resuability in case someone expects/needs an OkHttp Response and is instead given my custom class.

Overall, I will probably just have to experiment and try out this solution to see how bad it is. Maybe further along down the road I'll find a better solution.

Currently, exceptions are just punted up to the caller to keep it simple to iterate on.
The main exception is the IOException that we throw when an HTTP request is not successful. Also, our handling of the exception is not uniform across the api (i.e. in the SSEClient listener, we handle the exception ourselves. this may or may not be valid)
However, I feel that it could be improved upon. Maybe a custom exception unique to the Airlock implementation (such as ShipUnreachable or ConnectionError) could be more appropriate. A good design remains to be seen.

Currently, there are two areas which can be considered wasteful.

The first is that we create a deepcopy of each object sent to us in the Urbit.sendJSONtoChannel function. This could be wasteful and improved upon.

The second is that, for simplicity, we simply synchronize the whole of the sendJSONtoChannel and onEvent methods to ensure proper acking behaviour. At the cost of time/potential multi-threading bugs, we could try to synchronize only parts of the code which require it, and let the thread-safe/independent code run unfettered.

Versioned pokes are here, and unversioned pokes will be deprecated soon. We'll need to update the code to reflect that

Given a fixed set of actions (packets sent to eyre), the response should also be fixed. Currently, however, multiple runs of the same exact code can net different network activity, i.e., different packets received from the ship.

After some experimentation, I'm lead to believe that the root cause is because the message sending code runs on the main thread (sendJSONtoChannel) and the eventListener code that handles SSE events runs on a separate thread (b/c the okhttp api is designed this way).

I'm gonna study how the go library does it cause it might have a solution (it uses locks i think) so basically I'll have to try synchronize stuff between the threads (yikes). Remains to be seen how it will pan out.

the first block seems to always be 0, i.e 0v1.0.aaaaa

There were some breaking changes iirc to graph store that this library's agent does not support. We need to take a look at that eventually. I think they were related to pending messages.

Currently, all network requests are made synchronously, but it may be desirable to make these async because of ergonomics in real world usage.

While running integration tests, the fakezod seems to be outputting a debug message like so:
[%e %clogged '<channel_name>' n]
[%e %clogged '1606850483506-65c308' 8]

This probably means we are not acking something or acking too much or something generally bad going on in the main client Urbit.

Investigate

Currently, there is duplicated code in the Urbit class which can be refactored out.

basically, this thing would be the java specific version of the setup_fakezod.sh script.

Currently, cookies or really just the authentication cookie) are stored manually as a string, and extracted/injected manually as well through manipulating request/response headers.

The more idiomatic way of doing this is to use a CookieJar, but examples and documentation on how to use it are sparse.

$todo: add more info$