Signals (part 1) #982
Signals (part 1) #982
Conversation
| Holo(JsonString), | ||
| } | ||
|
|
||
| impl Serialize for Signal { |
There was a problem hiding this comment.
Do we have to implement this by hand? If so, why do we?
There was a problem hiding this comment.
@sphinxc0re because we can't immediately derive Serialize for Action, which Signal::Internal contains. This is a temporary way to move forward without implementing Serialize for the very big Action enum.
Co-Authored-By: maackle <maackle.d@gmail.com>
There was a problem hiding this comment.
My big question is where does the need for this arise?
"https://github.com/holochain/jsonrpc", branch = "broadcaster-getter"
and what is the work of upkeep for that?
| @@ -23,8 +23,9 @@ serde_json = { version = "1.0", features = ["preserve_order"] } | |||
| toml = "0.4.8" | |||
| boolinator = "2.4" | |||
| tiny_http = "0.6.0" | |||
| jsonrpc-ws-server = { git = "https://github.com/paritytech/jsonrpc" } | |||
| jsonrpc-http-server = { git = "https://github.com/paritytech/jsonrpc" } | |||
| jsonrpc-core = { git = "https://github.com/holochain/jsonrpc", branch = "broadcaster-getter" } | |||
There was a problem hiding this comment.
what's the fork for?
There was a problem hiding this comment.
There was a problem hiding this comment.
We'll have to remember to keep this fork up to date with jsonrpc upstream
core/src/instance.rs
Outdated
| @@ -197,7 +197,7 @@ impl Instance { | |||
| *state = new_state; | |||
| } | |||
|
|
|||
| // context.log(format!("trace/reduce: {:?}", action_wrapper.action())); | |||
| context.log(format!("trace/reduce: {:?}", action_wrapper.action())); | |||
There was a problem hiding this comment.
Are you playing tag on this line of code? Didn't we uncomment it once now it's recommented and now you're uncommenting it again?
There was a problem hiding this comment.
I added an "exclude: ^trace/" rule to LogRules::default, which will allow this line to stay in, what do you think @Connoropolous @zippy ?
| // @TODO: if needed for performance, could add a filter predicate here | ||
| // to prevent emitting too many unneeded signals | ||
| let signal = Signal::Internal(action); | ||
| tx.send(signal).unwrap_or(()) | ||
| // @TODO: once logging is implemented, kick out a warning for SendErrors | ||
| tx.send(signal).unwrap_or_else(|_e| { |
conductor_api/src/conductor/base.rs
Outdated
| let _ = log_sender.send((String::from("conductor"), message)); | ||
| error | ||
| }) | ||
| let (kill_switch_tx, kill_switch_rx) = channel(); |
There was a problem hiding this comment.
can you say something (and possibly in a code comment documented) about the kill_switch? where/when does it get called?
There was a problem hiding this comment.
Nothing new here, just gave it a more descriptive name. I'll find a place to add a comment.
| .start_http(&url.parse().expect("Invalid URL!")) | ||
| .map_err(|e| e.to_string())?; | ||
| let _ = kill_switch.recv(); | ||
| Ok(()) | ||
| let broadcaster = Broadcaster::Noop; |
There was a problem hiding this comment.
So HTTP implements a broadcaster but it's really just a no-op. So you've also set up a pattern that's extendable into different interfaces being implemented...
There was a problem hiding this comment.
This sounds right. @maackle it seems like this same method could be used to implement a LongPollHttpInterface that could do broadcasting over http, it just waits to "send" the messages when it gets a poll http request.
There was a problem hiding this comment.
Exactly. Each new Interface has the opportunity to also specify a Broadcaster, or use Noop if it doesn't support two-way messaging.
There was a problem hiding this comment.
You could use it to do HTTP/2 server-sent events too...
conductor_api/src/conductor/base.rs
Outdated
| } | ||
|
|
||
| pub fn config(&self) -> Configuration { | ||
| self.config.clone() | ||
| } | ||
|
|
||
| pub fn start_signal_broadcast(&mut self, signal_rx: SignalReceiver) -> thread::JoinHandle<()> { |
There was a problem hiding this comment.
this function I think needs some rusdoc comments because it's a pub function and feels like an important place where people will look.
| .start_http(&url.parse().expect("Invalid URL!")) | ||
| .map_err(|e| e.to_string())?; | ||
| let _ = kill_switch.recv(); | ||
| Ok(()) | ||
| let broadcaster = Broadcaster::Noop; |
There was a problem hiding this comment.
This sounds right. @maackle it seems like this same method could be used to implement a LongPollHttpInterface that could do broadcasting over http, it just waits to "send" the messages when it gets a poll http request.
| @@ -222,6 +252,7 @@ impl Conductor { | |||
| } | |||
|
|
|||
| /// Stop and clear all instances | |||
| /// @QUESTION: why don't we care about errors on shutdown? | |||
There was a problem hiding this comment.
@lucksus I put this in randomly to remind me to ask: I remember this got changed in a PR, but it feels wrong, why are we ignoring the possibility of shutdown errors?
Co-Authored-By: maackle <maackle.d@gmail.com>
|
@Connoropolous, the unit under test would be the code that facilitates sending data "backwards" through the websocket. We could test that, as I said, by spawning a conductor and talking to it via a websocket. I actually added some more commits in preparation for writing such a test but I'm a bit stuck finding the right websocket client crate that we could use in such a test. IMHO it would make more sense to test that from JS, which is exactly what we want to do and what this is a preparation step for. |
| @@ -968,7 +971,11 @@ impl ConductorApiBuilder { | |||
| } | |||
|
|
|||
| pub trait Interface { | |||
| Err("Error while trying to create instance \"test-instance-1\": Key from file \'holo_tester1.key\' (\'HcSCI7T6wQ5t4nffbjtUk98Dy9fa79Ds6Uzg8nZt8Fyko46ikQvNwfoCfnpuy7z\') does not match public address HoloTester1-----------------------------------------------------------------------AAACZp4xHB mentioned in config!" | ||
| .to_string()), | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_signals_through_admin_websocket() { |
There was a problem hiding this comment.
@Connoropolous, I've added this test that shows signals received via websocket.
|
Oh good!
…On Fri, Apr 12, 2019 at 5:35 PM Michael Dougherty ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In core/src/signal.rs
<#982 (comment)>
:
> + fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+ where
+ S: Serializer,
+ {
+ match self {
+ Signal::Internal(_) => serializer.serialize_newtype_variant(
+ "Signal",
+ 0,
+ "Internal",
+ "(Internal signal serialization not yet implemented)",
+ ),
+ Signal::User(msg) => {
+ serializer.serialize_newtype_variant("Signal", 1, "User", &msg.to_string())
+ }
+ Signal::Holo(msg) => {
+ serializer.serialize_newtype_variant("Signal", 2, "Holo", &msg.to_string())
Good catch, this can be removed. It was from back when I was assuming that
the wormhole hack would be implemented via Signals
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#982 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABWAYXNT2y9wjhwAvnD94H9MohzI31Vfks5vgPwmgaJpZM4azMj5>
.
|
This adds a notion of
Broadcaster, which is something that knows how to send aSignalback to the client. Each Interface implementation'srunmethod returns aBroadcaster. Then, if the Conductor is set up for it, it will start a new thread which continually consumes the signal channel and sends each signal over every interface via its Broadcaster.In order to maintain backwards compatibility with nodejs_conductor, the conductor can be configured in one of three states, with regard to signals:
This PR also includes a new module
core_types::ugly, which contains a typeInitable, representing a value that may or may not be initialized. It is isomorphic toOption. With this type, we can better keep track of which values are truly Optional, and which ones we just used Option for to represent invalid or uninitialized state.Question
Seems that signals need to be serializable (into JsonString), and currently Action is a big ol' non-serializable struct which gets passed as a signal. Should we bite the bullet and actually implement Serialize for Action and allll its sub-structs, or somehow punt on that since we don't need that for Holo?
If we do want
Actionto be serializable eventually, let's do it now. Otherwise, we can punt, but that would also be a bit of work, probably meaning we have to manually implementSerializefor Signal and just return some dummy value for the case ofSignal::Internal.I have added a summary of my changes to the changelogno new user-facing functionality, only groundworkNico's additions
fixes #739