Tracing (Internal) Signals #732

maackle · 2018-12-06T23:58:54Z

Wrap Instance and Context signal senders in Option rather than initializing with broken channels
Add Signal struct with Internal variant
Emit Signal::Internal for most Actions (all but InitApplication)
Test for said emitting
Adds utility for replacing history.len() checks with a signal receiver

- Wrap signal senders in Option rather than initializing with broken channels - Add Signal struct with Internal variant - Emit Signal::Internal for most Actions (all but InitApplication) - Test for said emitting - Adds utility for replacing history.len() checks with a signal receiver

core/src/instance.rs

core/src/nucleus/actions/build_validation_package.rs

core/src/instance.rs

zippy

Nice work! This is great stuff. Approved but I would like to see the printlns removed...

zippy · 2018-12-13T22:56:28Z

container/src/main.rs

@@ -72,7 +72,11 @@ fn bootstrap_from_config(path: &str) -> Result<Container, HolochainError> {
    config
        .check_consistency()
        .map_err(|string| HolochainError::ConfigError(string))?;
-    Container::try_from(&config)
+    let mut container = Container::from_config(config).with_signal_handler(|sig| {
+        println!("WE GET SIGNAL:\n{:?}", sig);


not sure I like the println here. I think this should be moved into a bonafide test.

zippy · 2018-12-13T22:58:37Z

container_api/src/container.rs

+            });
+            self.signal_thread = Some(signal_thread);
+        } else {
+            println!("Warning: no signal handler was set!");


println also, perhaps this function should return a Result?

zippy · 2018-12-13T23:00:53Z

container_api/src/container.rs

+            // @NB: if the Instance constructor ever changes such that it can accept a SyncSender rather than
+            // spitting out a Receiver, we can just clone that Sender instead of gathering the receivers
+            let signal_rx = combine_receivers(signal_rxs);
+            self.spawn_signal_thread(signal_rx);


if spawn_signal_thread returns a Result then this could just have a ?

zippy · 2018-12-13T23:05:30Z

core/src/context.rs

+    // @NB: these three getters smell bad because previously Instance and Context had SyncSenders
+    // rather than Option<SyncSenders>, but these would be initialized by default to broken channels
+    // which would panic if `send` was called upon them. These `expect`s just bring more visibility to
+    // that potential failure mode.


maackle · 2018-12-14T03:25:22Z

I realized that specifying a signal handler in the container is a bit overkill. It's far simpler to set up a channel outside the Container and pass in the SyncSender, and let the Container invoker do whatever they want with the receiver, including possibly creating an hander loop. This also obviates the need to combine the receivers from each Instance (which required spinning up a bunch of extra threads), since now the SyncSender can just get cloned for each new Instance that needs to send a signal.

I would appreciate a look at the new changes @zippy and @lucksus.

codecov · 2018-12-14T03:36:26Z

Codecov Report

Merging #732 into develop will increase coverage by 0.31%.
The diff coverage is 85.49%.

@@             Coverage Diff             @@
##           develop     #732      +/-   ##
===========================================
+ Coverage    72.37%   72.67%   +0.31%     
===========================================
  Files          146      147       +1     
  Lines         4947     5005      +58     
===========================================
+ Hits          3580     3637      +57     
- Misses        1367     1368       +1

Impacted Files	Coverage Δ
core/src/nucleus/actions/validate.rs	`73.34% <ø> (ø)`	⬆️
container_api/src/interface.rs	`68.75% <ø> (ø)`	⬆️
container/src/main.rs	`0% <ø> (ø)`	⬆️
...re/src/nucleus/actions/build_validation_package.rs	`69.45% <ø> (ø)`	⬆️
core/src/workflows/handle_custom_direct_message.rs	`0% <0%> (ø)`	⬆️
core/src/nucleus/ribosome/api/update_entry.rs	`0% <0%> (ø)`	⬆️
core/src/network/actions/custom_send.rs	`0% <0%> (ø)`	⬆️
core/src/nucleus/ribosome/api/remove_entry.rs	`0% <0%> (ø)`	⬆️
core/src/nucleus/ribosome/api/call.rs	`51.79% <0%> (ø)`	⬆️
core/src/dht/actions/hold.rs	`57.15% <100%> (ø)`	⬆️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a32b7db...51e1ff8. Read the comment docs.

codecov · 2018-12-14T03:36:26Z

Codecov Report

Merging #732 into develop will increase coverage by 0.3%.
The diff coverage is 85.49%.

@@            Coverage Diff             @@
##           develop     #732     +/-   ##
==========================================
+ Coverage    72.64%   72.94%   +0.3%     
==========================================
  Files          145      146      +1     
  Lines         4922     4980     +58     
==========================================
+ Hits          3575     3632     +57     
- Misses        1347     1348      +1

Impacted Files	Coverage Δ
core/src/nucleus/actions/validate.rs	`73.34% <ø> (ø)`	⬆️
container_api/src/interface.rs	`68.75% <ø> (ø)`	⬆️
container/src/main.rs	`0% <ø> (ø)`	⬆️
...re/src/nucleus/actions/build_validation_package.rs	`69.45% <ø> (ø)`	⬆️
core/src/workflows/handle_custom_direct_message.rs	`0% <0%> (ø)`	⬆️
core/src/nucleus/ribosome/api/update_entry.rs	`0% <0%> (ø)`	⬆️
core/src/network/actions/custom_send.rs	`0% <0%> (ø)`	⬆️
core/src/nucleus/ribosome/api/remove_entry.rs	`0% <0%> (ø)`	⬆️
core/src/nucleus/ribosome/api/call.rs	`51.79% <0%> (ø)`	⬆️
core/src/dht/actions/hold.rs	`61.91% <100%> (+4.77%)`	⬆️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d90b968...c48cc35. Read the comment docs.

lucksus

Still good :)
I started refactoring Context and adding a builder. I should base my stuff off of this. Either we merge this soon or I will just merge this into my branch...

zippy · 2018-12-14T18:36:50Z

I'm mildly in favor of just merging this before we figure out the windows problem and adding it to the list. However I did notice something weird. Look at line 848 of https://travis-ci.com/holochain/holochain-rust/jobs/165253486

why are there those compiler warnings? I don't see them when I run holochain-cmd tests locally.

maackle added 2 commits December 6, 2018 18:55

fmt

777ec5e

Connoropolous added the review label Dec 6, 2018

Refine expect_action and move to test_utils

29a9b97

maackle mentioned this pull request Dec 7, 2018

abstraction for inspecting the state history for specific actions, don't inspect history state #195

Open

maackle added 11 commits December 7, 2018 10:44

WIP hook up signal handler to container

d0c46fd

Merge remote-tracking branch 'origin/develop' into signals-internal

c8af08c

WIP

964a5ed

Expose SyncSender rather than Option<SyncSender>

d6b7277

A whole lot of println!

46f9b50

Merge remote-tracking branch 'origin/develop' into signals-internal

ca7ec93

Merge remote-tracking branch 'origin/develop' into signals-internal

c76274e

Fixes for new merged code

0d38310

Change container construction process

79d3252

Some debug cleanup

da9d93b

Update tests

45027bf

maackle force-pushed the signals-internal branch from 98fd131 to 45027bf Compare December 11, 2018 01:05

maackle changed the title ~~[WIP] Signals internal~~ Tracing (Internal) Signals Dec 11, 2018

lucksus approved these changes Dec 12, 2018

View reviewed changes

core/src/instance.rs Outdated Show resolved Hide resolved

core/src/nucleus/actions/build_validation_package.rs Outdated Show resolved Hide resolved

lucksus reviewed Dec 12, 2018

View reviewed changes

core/src/instance.rs Outdated Show resolved Hide resolved

lucksus reviewed Dec 12, 2018

View reviewed changes

core/src/instance.rs Show resolved Hide resolved

lucksus reviewed Dec 12, 2018

View reviewed changes

core/src/instance.rs Outdated Show resolved Hide resolved

maackle added 3 commits December 12, 2018 11:55

Fix tests, PR feedback

e164656

Add predicate arg to with_signals for filtering

c6a3201

Merge remote-tracking branch 'origin/develop' into signals-internal

934f555

maackle requested review from thedavidmeister and zippy December 12, 2018 21:40

Merge remote-tracking branch 'origin/develop' into signals-internal

86197db

zippy approved these changes Dec 13, 2018

View reviewed changes

action_channel() cleanup

854d3c8

maackle added 6 commits December 13, 2018 15:45

Override DnaWasm Debug impl

b754fbb

Add timeouts to signal checking tests

bb8c7a1

Remove unnecessary unsafe impl!

a3def70

Simplify signal channel setup by providing Sender

22cdbc6

Merge remote-tracking branch 'origin/develop' into signals-internal

7b3a1a7

Add test for Container::with_signal_channel

745263a

Remove signal handler reference in container

c48cc35

maackle added 2 commits December 13, 2018 22:44

Merge branch 'develop' into signals-internal

d9e24d7

Merge remote-tracking branch 'origin/develop' into signals-internal

c797ec8

lucksus approved these changes Dec 14, 2018

View reviewed changes

Uniquify test DNA

51e1ff8

lucksus merged commit db7117f into develop Dec 14, 2018

lucksus removed the review label Dec 14, 2018

maackle deleted the signals-internal branch December 14, 2018 20:21

maackle mentioned this pull request Dec 15, 2018

Signals bug fix #764

Merged

thedavidmeister mentioned this pull request Jan 7, 2019

SPIKE: 2019 01 04 codecov returns #813

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracing (Internal) Signals #732

Tracing (Internal) Signals #732

maackle commented Dec 6, 2018

zippy left a comment •

edited

Loading

zippy Dec 13, 2018

zippy Dec 13, 2018

zippy Dec 13, 2018

zippy Dec 13, 2018

maackle commented Dec 14, 2018 •

edited

Loading

codecov bot commented Dec 14, 2018 •

edited

Loading

codecov bot commented Dec 14, 2018

lucksus left a comment

zippy commented Dec 14, 2018

Tracing (Internal) Signals #732

Tracing (Internal) Signals #732

Conversation

maackle commented Dec 6, 2018

zippy left a comment • edited Loading

Choose a reason for hiding this comment

zippy Dec 13, 2018

Choose a reason for hiding this comment

zippy Dec 13, 2018

Choose a reason for hiding this comment

zippy Dec 13, 2018

Choose a reason for hiding this comment

zippy Dec 13, 2018

Choose a reason for hiding this comment

maackle commented Dec 14, 2018 • edited Loading

codecov bot commented Dec 14, 2018 • edited Loading

Codecov Report

codecov bot commented Dec 14, 2018

Codecov Report

lucksus left a comment

Choose a reason for hiding this comment

zippy commented Dec 14, 2018

zippy left a comment •

edited

Loading

maackle commented Dec 14, 2018 •

edited

Loading

codecov bot commented Dec 14, 2018 •

edited

Loading