Bridge API #776

lucksus · 2018-12-18T14:01:47Z

Context

This is about exposing the container API to instances for the purpose of bridging.
This SoA in the tree:

`ContainerApiDispatcher` -> Builder

The main changes here are to what was ContainerApiDispatcher before and is now ContainerApiBuilder. The main thing used outside was the IoHandler anyway. I am using the same thing that gets exposed to (external) interfaces now also for interfacing from instances. But for that we need more control over the methods that get registered in a particular interface context. Hence the transformation to a builder.

`Container::instantiate_from_config()`

That function moved into Container and makes use of the ContainerApiBuilder to add each instance that the current instance depends on. This relies on my last PR #773 that makes sure that we have a topological ordering of instances so we can expect dependencies to have spawned before. (See those expect() statements there).

Changes around `Context`

Since I had to change Context::new again to include the container_api: IoHandler I couldn't resist but apply the ContextBuilder in all those places that I had to touch again. Sorry for the noise.

…ible API to parameterize constructed interface intended for bridging

…ntextBuilder in lots of places, where possible (added ContextBuilder::with_logger)

…ntexts. (instantiate_from_config() is tested via container.load_config)

StaticallyTypedAnxiety · 2018-12-18T18:03:16Z

container_api/src/container.rs

+                    let callee_config = config.instance_by_id(&bridge.callee_id)
+                        .expect("config.check_consistency()? jumps out if config is broken");
+                    let callee_instance = self.instances.get(&bridge.callee_id)
+                        .expect(r#"


If we are returning a result, do we need these expects? same question of asserts

If those expectations aren't met, it would not be a runtime error but a fundamental mistake in this approach. I assert that this can never happen with the current code. If it would we need to learn about it asap and fix it. If this panics because of changes to the code base I would want the developer to introduce changes to get drawn to this code and see what's going on.

maackle

I like the simplification of passing around a bare IoHandler rather than trying to clothe it in another type, I was thinking of doing that too but just wanted to get it out the door so I'm glad you had occasion to touch it and fix that.

Looks good overall but do you think the ContainerApiBuilder is perhaps a bit too expressive? How often will somebody need to add a single named instance, à la carte? The concern I have with this is that the instances and the instance configs are stored in separate HashMaps which can contradict each other since they can be built up separately. This line would then lead to silent failure.

If I'm reading correctly I would rather see the instances and their configs more tightly coupled, either by requiring instance and config to be passed together, or even having a single HashMap with tuple values for both instance and config. What do you think about this concern?

container_api/src/container.rs

maackle · 2018-12-18T18:35:24Z

container_api/src/container.rs

+
+    /// Creates one specific Holochain instance from a given Configuration,
+    /// id string and DnaLoader.
+    pub fn instantiate_from_config(


Curious why you moved this inside as a method?

Because it now depends on the Container's instances since instances depend on other instances through bridging. Leaving it outside would mean more mutable parameters which all get set to some self.xxx when called from a container instance method. That resulted in some problems with the borrow checker because of several mutable borrows to self.

container_api/src/context_builder.rs

maackle · 2018-12-18T18:44:55Z

container_api/src/interface.rs

+    /// Add a [InstanceConfig](struct.InstanceConfig.html) for a custom named instance
+    pub fn with_named_instance_config(
+        mut self,
+        instance_name: String,


I was confused by seeing instance_name until I looked around and saw that it's the instance ID from the config, so why not leave it instance_id? Seems like this is introducing an idea of a "named instance", does that name have any significance outside of the config?

Yes, that is exactly what is needed for bridging and what is used in this PR. In Container::instantiate_from_config() this function is called and the instance_name is set to the handle as defined in the bridge. The DNA caller code should not depend on a container's config but instead defines a handle for its bridges. This is the main reason I had to break open ContainerApiDispatcher actually.

maackle · 2018-12-18T18:57:14Z

container_api/src/interface.rs

            .instances
            .iter()
-            .map(|inst| (inst.id.clone(), inst.clone()))
+            .filter(|&(name, _)| instance_configs.contains_key(name))


Isn't it kind of a big deal if the instances and instance_configs hashmaps don't have the same keys? See my overall comment.

Hm, not really because the configs are only used in the "info/instances" method. For bridging we could actually leave out the configs alltogether..

The info/instances is how the UI knows what DNA hashes it can refer to, pretty essential if the UI hard-codes DNA hashes as the fundamental address of a DNA, which I think it should. So if someone writes a container that doesn't pass an config for every instance, it will break the UI.

lucksus · 2018-12-18T19:40:41Z

@maackle Thanks for the thorough response!

We hand-pick instances in this line with different names than the instance ID in the config. And this is basically the heart of this PR: making available an instance to another one under a name chosen a priori (in the DNA, like here: #752) by the caller.

I'm not super concerned about the config/instance drift since the config is not needed in all cases, like the one implemented here. The result is just that it is not listed in the info output. And maybe that is something we even would want to make possible for certain cases. If it is important for your use-case to have all instances listed there, make sure you provide configs for all.. I kinda prefer having the builder be flexible enough to cater for these different use-cases. Don't you think?

maackle · 2018-12-18T20:00:51Z

@lucksus I hadn't considered the possibility of intentionally hiding instances from info/instances (and hence the UI) by not passing configs, but you're right that not every DNA needs to be externally visible. My assumption was that configs and instances would always be one-to-one. But it sounds like we do always want configs to be a subset of instances (we never want a config for a non-existent instance) so what about this?

fn with_named_instance(&self, String, Arc<RwLock<Holochain>>);
fn with_named_instance_and_config(&self, String, (Arc<RwLock<Holochain>>, InstanceConfiguration));
fn with_instances(mut self, instances: HashMap<String, Arc<RwLock<Holochain>>>);
fn with_instance_config_pairs(mut self, pairs: HashMap<String, (Arc<RwLock<Holochain>>, InstanceConfiguration)>)

Then you can never pass a config without an instance, but you can pass bare instances. It could be a follow up PR but what do you think? That enforces "configs are a subset of instances". I'm just a huge fan of using the type system to enforce invariants and make unwanted states impossible.

zippy

just some comments for clarity. Nice work!

zippy · 2018-12-19T04:45:25Z

container_api/src/container.rs

+                        .expect("config.check_consistency()? jumps out if config is broken");
+                    let callee_instance = self.instances.get(&bridge.callee_id).expect(
+                        r#"
+                            We have to create instances ordered by bridge dependencies such that we


really? This is a pretty crazy expectation, sounds more like an error message that would be reported to the user or logged? Or am I missing something...

I'm saying the description that this relies on my previous work in #773. Instantiate from config is called in this loop over instances sorted by bridge dependencies.

Am I wrong in making this an explicit invariant? The ContainerApiBuilder needs live instances to reference in the closures that get registered in the IoHandler. I mean, that could be changed, but then there will be bridge genesis that would happen in Holochain::new a few lines below. At that point we need the instances that this instance depends to be up an running, no?

So currently, this code does enforce this expectation and making it explicit in this expect feels like the right way to communicate it to our future selves looking at the code.. ;)

ok. yep, got it

zippy · 2018-12-19T04:49:56Z

container_api/src/context_builder.rs

    //persister: Option<Arc<Mutex<Persister>>>,
    chain_storage: Option<Arc<RwLock<ContentAddressableStorage>>>,
    dht_storage: Option<Arc<RwLock<ContentAddressableStorage>>>,
    eav_storage: Option<Arc<RwLock<EntityAttributeValueStorage>>>,
    network_config: Option<JsonString>,
+    container_api: Option<Arc<RwLock<IoHandler>>>,


why is this called a "container_api" that seems like a weird name. I see that the IoHandler is returned by make_interface_handler i.e to a specific channel that can be connected on for external calls, so I'm confused to see it called a container_api here.

Well, it is a "container API". I am using the ContainerApiBuilder to register different methods for external interfaces in make_interface_hander vs internal interfaces, i.e. bridges, that are passed into the instances. Here I'm setting this container_api with the version for bridges: https://github.com/holochain/holochain-rust/pull/776/files#diff-9f381fb2edab09fcf479f47ae307a2dbR136.

In yesterday's call I suggested to @maackle that signals could work over the same container API just by using a (yet to be added) builder function ContainerApiBuilder::with_signal_functions().

The idea is that this container API is the only thing we need to pass into the instances for all kinds of callbacks. It is the interface the container exposes to instances which I'm sure will grow in the future.

zippy · 2018-12-19T04:53:37Z

container_api/src/holochain.rs

-                mock_network_config(),
-            )),
+            Arc::new(
+                ContextBuilder::new()


nice pattern!

lucksus · 2018-12-19T12:28:36Z

@zippy, I've responded to your comments. I'm waiting for you to read and maybe respond before merging. If you're fine with this please go ahead and hit the merge button :)

lucksus added 3 commits December 17, 2018 23:27

Configuration::bridge_dependencies()

a901d12

Merge branch 'develop' into bridge-api

95a9c78

Transformed ContainerApiDispatcher into ContainerApiBuilder with flex…

18ede15

…ible API to parameterize constructed interface intended for bridging

lucksus added the review label Dec 18, 2018

lucksus added 4 commits December 18, 2018 16:22

ContainerApiBuilder rustdocs

f364e31

Add container_api to Context -> change Context::new() -> switch to Co…

48778db

…ntextBuilder in lots of places, where possible (added ContextBuilder::with_logger)

Remove failing rustdoc example

5133a50

Spawn instances in bridge order and build bridge APIs to pass into co…

55d87cd

…ntexts. (instantiate_from_config() is tested via container.load_config)

lucksus changed the title ~~WIP: Bridge API in container~~ Bridge API Dec 18, 2018

lucksus requested review from maackle, zippy, StaticallyTypedAnxiety, Connoropolous, sphinxc0re and thedavidmeister December 18, 2018 17:55

rustfmt

1b689b2

StaticallyTypedAnxiety reviewed Dec 18, 2018

View reviewed changes

Bridge::handle

b314282

maackle suggested changes Dec 18, 2018

View reviewed changes

lucksus added 2 commits December 18, 2018 20:43

Make Container::config private

e23c780

Update comment in ContextBuilder

279d61c

Merge branch 'develop' into bridge-api

ad7b58f

maackle approved these changes Dec 18, 2018

View reviewed changes

maackle mentioned this pull request Dec 18, 2018

Update ContainerApiBuilder to enforce assumptions #777

Open

StaticallyTypedAnxiety approved these changes Dec 19, 2018

View reviewed changes

zippy approved these changes Dec 19, 2018

View reviewed changes

Merge branch 'develop' into bridge-api

164c0a6

lucksus merged commit b921eab into develop Dec 19, 2018

lucksus removed the review label Dec 19, 2018

zippy deleted the bridge-api branch July 5, 2019 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bridge API #776

Bridge API #776

lucksus commented Dec 18, 2018 •

edited

Loading

StaticallyTypedAnxiety Dec 18, 2018

lucksus Dec 18, 2018

maackle left a comment •

edited

Loading

maackle Dec 18, 2018

lucksus Dec 18, 2018

maackle Dec 18, 2018

lucksus Dec 18, 2018

maackle Dec 18, 2018

lucksus Dec 18, 2018

maackle Dec 18, 2018

lucksus commented Dec 18, 2018

maackle commented Dec 18, 2018

zippy left a comment

zippy Dec 19, 2018

lucksus Dec 19, 2018

zippy Dec 19, 2018

zippy Dec 19, 2018

lucksus Dec 19, 2018

zippy Dec 19, 2018

zippy Dec 19, 2018

lucksus commented Dec 19, 2018

Bridge API #776

Bridge API #776

Conversation

lucksus commented Dec 18, 2018 • edited Loading

Context

ContainerApiDispatcher -> Builder

Container::instantiate_from_config()

Changes around Context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maackle left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucksus commented Dec 18, 2018

maackle commented Dec 18, 2018

zippy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucksus commented Dec 19, 2018

lucksus commented Dec 18, 2018 •

edited

Loading

`ContainerApiDispatcher` -> Builder

`Container::instantiate_from_config()`

Changes around `Context`

maackle left a comment •

edited

Loading