GH-678: Create a Message Scheduler #249
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…o differentiate from the other
…tream_handler_pool.rs
bertllll
reviewed
Mar 30, 2023
masq_lib/src/messages.rs
Outdated
| @@ -843,6 +845,12 @@ pub struct UiWalletAddressesResponse { | |||
| } | |||
| conversation_message!(UiWalletAddressesResponse, "walletAddresses"); | |||
|
|
|||
| #[derive(Message, Clone, PartialEq, Eq)] | |||
There was a problem hiding this comment.
Look at the other message in this file and notice that you don't find any Actor messages here. This file is exclusively intended for Websocket messages between UI and Node or Daemon.
Usually the place you might have been looking for in masq_lib is considered the file utils.rs. However, I don't think we want to go there either. Think about what this whole library serves for, it is supposed to offer code that can occur in both, the frontend side (UI) and the backend side.
In this case it doesn't make sense to put it there because the UI doesn't use Actors at all... and it is of no use for it then.
Please find a nice location in the node crate. I'm suggesting /sub_lib/utils.rs as probably the best one.
masq_lib/src/messages.rs
Outdated
| @@ -843,6 +845,12 @@ pub struct UiWalletAddressesResponse { | |||
| } | |||
| conversation_message!(UiWalletAddressesResponse, "walletAddresses"); | |||
|
|
|||
| #[derive(Message, Clone, PartialEq, Eq)] | |||
| pub struct MessageScheduler<M: Message> { | |||
| pub schedule_msg: M, | |||
There was a problem hiding this comment.
maybe scheduled_msg instead?
masq_lib/src/messages.rs
Outdated
| #[derive(Message, Clone, PartialEq, Eq)] | ||
| pub struct MessageScheduler<M: Message> { | ||
| pub schedule_msg: M, | ||
| pub duration: Duration, |
There was a problem hiding this comment.
Instead of repeating the word duration, the filed could say something like msg_delay
There was a problem hiding this comment.
Instead, I just kept it to delay.
node/src/node_test_utils.rs
Outdated
| @@ -307,6 +308,7 @@ pub fn make_stream_handler_pool_subs_from_recorder(addr: &Addr<Recorder>) -> Str | |||
| bind: recipient!(addr, PoolBindMessage), | |||
| node_query_response: recipient!(addr, DispatcherNodeQueryResponse), | |||
| node_from_ui_sub: recipient!(addr, NodeFromUiMessage), | |||
| schedule_message_sub: recipient!(addr, MessageScheduler<DispatcherNodeQueryResponse>), | |||
There was a problem hiding this comment.
This is not actually just scheduled_msg it should rather be scheduled_node_query_response_sub. Keep in mind that theoretically you could have multiple scheduled messages and the recipients for them in the same actor. So mentioning the message kind is important (by the way, it would be handy to be able to read it even if we were clearly sure it's gonna be the only scheduled message ever in this place).
node/src/stream_handler_pool.rs
Outdated
| @@ -62,6 +62,7 @@ pub struct StreamHandlerPoolSubs { | |||
| pub bind: Recipient<PoolBindMessage>, | |||
| pub node_query_response: Recipient<DispatcherNodeQueryResponse>, | |||
| pub node_from_ui_sub: Recipient<NodeFromUiMessage>, | |||
| pub schedule_message_sub: Recipient<MessageScheduler<DispatcherNodeQueryResponse>>, | |||
| @@ -155,6 +157,17 @@ impl Handler<DispatcherNodeQueryResponse> for StreamHandlerPool { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This thing below is all good. My only request is, if you haven't done yet, please create a card with tiny description about implementing this through a derive macro.
Also, I'd prefer to have a simple TODO here referring to the newly existing card.
There was a problem hiding this comment.
I've created the card, and marked a TODO referencing it - MASQ-Project/MASQ-Node-issues#686
node/src/stream_handler_pool.rs
Outdated
| @@ -533,18 +550,19 @@ impl StreamHandlerPool { | |||
| peer_addr, | |||
| msg.context.data.len() | |||
| ); | |||
| let recipient = self | |||
| let schedule_message_sub = self | |||
There was a problem hiding this comment.
I think we need to call these variables rather scheduled_message_sub as you hardly can have a recipient for a wide range of messages (even if as a generic argument in the MessageScheduler).
node/src/test_utils/recorder.rs
Outdated
| @@ -92,6 +93,17 @@ macro_rules! recorder_message_handler { | |||
| }; | |||
| } | |||
|
|
|||
| impl<M> Handler<MessageScheduler<M>> for Recorder | |||
There was a problem hiding this comment.
I feel like this place is too prominent for this rather marginal thing. I'm asking you to move the implementation under all those recorder_message_handler macro calls.
* feat: migrate MessageScheduler to the sub_lib/utils.rs * feat: rename to scheduled_msg * GH-678: rename duration to delay * GH-678: rename the name of the sub * GH-678: add a todo for the procedural macro card * GH-678: rename to scheduled_node_query_response_sub * GH-678: migrate the MessageScheduler handler for Recorder after the recorder_message_handler implementaions
bertllll
approved these changes
Mar 31, 2023
FinsaasGH
added a commit
that referenced
this pull request
Apr 30, 2023
* GH-541 - Isolate Database-Migration Code (#237) * GH-541: finished code migration, tests pasing * GH-541: completed and ci/all tests passed * Added review changes - all test passing * GH-541-review-1: Resolved review comments from db_migrator.rs * GH-541 review 3 (#250) * GH-633: Optimise routing to reduce DNS resolution 'mark' (#183) * GH-633: modify how the NodeRecordMetadataMessage is handled * GH-633: use the hashset for unreachable hosts for compute_undesirability() * GH-633: NodeRecordMetadataMessage handler is able to add unreachable host to the hashset of unreachanle hosts * GH-633: add the ability to receive the Option of hostname inside compute_new_undesirability() * GH-633: remove clippy warnings * GH-633: fix tests in proxy_server.rs * GH-611: fix test masq_erc20_contract_exists_on_ethereum_ropsten_integration * GH-633: remove commented out code * GH-633: fix the errors in multinode integration tests * GH-633: use an enum to generalise NodeRecordMetadataMessage and only send this message when host name is found * GH-633: remove the desirable_for_exit field * GH-633: rename server_name to server_name_opt inside AddReturnRouteMessage * GH-633: remove problems due to change in NodeRecordMetadata structure inside multinode integration tests * GH-633: use lifetime specifier instead of cloning the hostname as a String * GH-633: Review 2 changes * GH-633: refactor the code isnide handle_dns_resolve_failure() * Partial multinode test * GH-633 Test is written, but doesn't pass yet * GH-633 New multinode test passes, but suspiciously * GH-633 Multinode test working, but failing; plus some light mods * Removed warnings * GH-499: Made ProxyServer picky about main and alias keys; fixed CryptDENull to panic about mismatched keys * There's something funky about the way Standard Gossip is processed that produces a half neighborship where there should be a full neighborship * GH-633: Test works better * GH-633: remove clippy warnings * GH-633: fix the failing test * Multinode test is still failing, but more successfully than ever before * GH-633: Commented out part of multinode test that doesn't pass in order to consider a new card * GH-633: minor review changes inside proxy_server/mod.rs * GH-633: formatting changes * GH-633: remove clippy warnings * GH-633: increase version * GH-633: rename field to originator_public_key * GH-631: Did more fixing on the blockchain multinode test * GH-633: An attempt to remove the warnings inside the multinode test * GH-633: change the version number of libc from 2.36-4 to 2.36-6 * GH-633: implement alias_public_key () and absorb_configuration() for MASQRealNode * GH-633: trying to fix the multinode test errors * GH-633: rename the field back to originator_alias_public_key * GH-633: remove clippy warnings from the multinode tests * GH-611: remove unused import from the file communication_failure_test.rs * GH-633: remove warnings from the connection_termination_test.rs * GH-633: Added some time for example.com to respond * GH-633: fixing to right use of cryptdes on decrypting cores packages as the routing service * GH-633: fix one test in the routing_service.rs * GH-633: modify helper functions and fix the test logs_and_ignores_message_that_cannot_be_decrypted * GH-633: fix test route_logs_and_ignores_cores_package_that_demands_proxy_client_routing_with_paying_wallet_that_cant_pay * GH-633: fix test converts_live_message_to_expired_for_existing_proxy_client * GH-633: continue with Bert * GH-633: add some panics and eprintlns for debugging * GH-633: fix all tests in routing_service.rs * GH-633: add a todo * GH-633: fix the integration test * GH-633: fix test reported_server_drop * GH-633: fix of an issue with fatal instance dropping: seeking for a decent solution of that need for allowence of post-creation configuration of MASQMockNode * GH-633(for bert): Review 1 Changes (#228) * GH-633: fix the test symmetric_encryption_fails_with_different_keys (only failing on IDE) * GH-633: remove the to_hex function * GH-633: reduce the thread sleep by 1 sec inside the bookkeeping test * GH-633: remove the lazy_static inside the test module of the file routing_service.rs * GH-633: review changes for the routing_service.rs * GH-633: add test route_data_to_peripheral_component_uses_main_key_on_payload_for_hopper * GH-633: add review changes for gossip_acceptor.rs * GH-633: further changes in gossip_acceptor.rs * GH-633: compose message only if log level is debug * GH-633: typecast payload_size to u64 * GH-633: improve the trace log for calculating undesirability of the DNS Resolution failure * GH-633: further review changes for the src/neighborhood/mod.rs * GH-633: join filter and map to form filter_map * GH-633: add diagram to the test standard_gossip_containing_unfamiliar_node_addrs_leads_to_them_being_ignored * GH-633: add review changes for the src/proxy_client/mod.rs * GH-633: rename variables in verify_sigature() * GH-633: use the 2 functions inside impl block of RatePack in production code * GH-633: bring back the test id_returns_error_when_the_id_fails_to_decrypt * GH-633: rename utility function to make_route_to_proxy_client() * GH-633: improve comment and rename variables in the function make_round_trip_route * GH-633: fix the problem with the match arm in compute_new_undesirability * GH-633: represent pub key in hex * GH-633: remove the commented out code after migrating it to a new card * GH-633: add assertions for dns error response inside the multinode test * GH-633: change host_name to hostname inside the NRMetadataChange::AddUnreachableHost * GH-633: formatting fixes * GH-633: make the assertions alive inside the multinode test and add a new todo * GH-633 (for bert): Review 2 Changes (#231) * GH-633: function rename to add_src_node_as_half_neighbor * GH-633: improve diagram for the test standard_gossip_containing_unfamiliar_node_addrs_leads_to_them_being_ignored * GH-633: improve the way debug message is built * GH-633: improve assertion for logs for fn computing_undesirability_works_for_exit_on_over_leg_for_blacklisted_host * GH-633: rename variables inside fn verify_signature * GH-633: rename the fn name to make_one_way_route_to_proxy_client * GH-633: formatiing changes * GH-633: remove blank line * GH-633: remove unnecessary comment inside the test * GH-633: attempt to fix build errors * GH-671: removing www.failingFailing.com as non-http anymore for www.neverssl.com * GH-633: fix formatting issues * GH-633: correction of a non-ideal design for a configurable mock node handle * GH-633: fix the test symmetric_encryption_fails_with_different_keys * GH-633: fix formatting issues * GH-633: review five (#235) * GH-633-after-review-four: addressed things from the review * GH-633: another try to implement the mutable MASQMockNode even better, huh * GH-633-after-review-four: a little change in function name --------- Co-authored-by: Bert <[email protected]> * GH-633: bump the version to 0.7.2 --------- Co-authored-by: Dan Wiebe <[email protected]> Co-authored-by: masqrauder <[email protected]> Co-authored-by: Bert <[email protected]> Co-authored-by: Bert <[email protected]> * Added review changes - all test passing * GH-679: Create a script for bumping version (#242) * initialize bump_version script * GH-679: add the ability to generate lockfile * GH-679: minor improvements * GH-679: add the ability to print the names of failed crates * GH-679: conditionally print the output message * GH-679: remove the error coming from the asterisk * GH-679: iterate through loops for the crates * GH-679: check if the right number of args were supplied * GH-679: quotes cleanup * GH-679: improve the regex * GH-679: decouple pattern from the sed command * GH-679: check the pattern with grep * GH-679: conditionally print message for failed crates * GH-679: migrate pushd and popd to the for loop * GH-679: add the ability to find crates * GH-679: review 2 changes * GH-541-review-1: Resolved review comments from db_migrator.rs * GH-544 - removing wrap_to_ok() and wrap_to_error() (#246) * Remove useless code utility #544 * replace wrap_to_ok() with Ok() in mock.rs line 626 * bumped port_exposer version * removing dead code and fix formatting --------- Co-authored-by: Vojtěch Parkán <[email protected]> * GH-677: Refactor the extremely long function `handle_dispatcher_node_query_response()` (#236) * Actor StreamHandlerPool and friends renamed to NeighborStreamHandlerPool * More cleanup renaming * One more... * code formatting * Big method is broken into three; needs more * Big match statement is broken into three methods. Possibly more to come. * Doesn't work; StreamStarter needs to be split and renamed * Closer, but still doesn't work * Make success and failure handler inside the open_new_stream_and_recycle_message * Modify the way we return results * Rename function name and variable * Changed an error message * Appeased clippy * Appeased formatter * master-commented: Corrected TODO comment * GH-677: add the comment over stream_handler_pool, to make it easier to differentiate from the other * GH-677: revert the rename to StreamHandlerPool * GH-677: revert the rename to StreamHandlerPoolSubs * GH-677: revert the rename to StreamHandlerPoolCluster * GH-677: revert the name from NeighborPublicKey to Key inside the enum Endpoint * GH-677: rename the filename from neighbor_stream_handler_pool.rs to stream_handler_pool.rs * GH-677: rename NeighborStreamHandlerPool to StreamHandlerPool at other places * GH-677: revert rename in the reamining places * GH-677: rename in crash_command.rs * GH-677: change the debug to an error * GH-677: remove the comment 'way to big' * GH-671: removing www.failingFailing.com as non-http anymore for www.neverssl.com * GH-677: fix formatting issue * GH-677: fix the multiple_stream_zero_hop_test * GH-677: wrote test log_an_error_when_it_fails_to_send_a_packet * GH-677: review 2 changes * GH-677: review 3 changes --------- Co-authored-by: Dan Wiebe <[email protected]> Co-authored-by: Bert <[email protected]> * GH-541-review-2: Finished review comments * GH-541-review-2: amended changes & increassed sleep duration * GH-541-review-2: Fix merge issues --------- Co-authored-by: Utkarsh Gupta <[email protected]> Co-authored-by: Dan Wiebe <[email protected]> Co-authored-by: masqrauder <[email protected]> Co-authored-by: Bert <[email protected]> Co-authored-by: Bert <[email protected]> Co-authored-by: Czarte <[email protected]> Co-authored-by: Vojtěch Parkán <[email protected]> * GH-541: deleted an empty file * GH-541: removed a println leftover --------- Co-authored-by: Utkarsh Gupta <[email protected]> Co-authored-by: Dan Wiebe <[email protected]> Co-authored-by: masqrauder <[email protected]> Co-authored-by: Bert <[email protected]> Co-authored-by: Bert <[email protected]> Co-authored-by: Czarte <[email protected]> Co-authored-by: Vojtěch Parkán <[email protected]> * GH-678: Create a Message Scheduler (#249) * Actor StreamHandlerPool and friends renamed to NeighborStreamHandlerPool * More cleanup renaming * One more... * code formatting * Big method is broken into three; needs more * Big match statement is broken into three methods. Possibly more to come. * Doesn't work; StreamStarter needs to be split and renamed * Closer, but still doesn't work * Make success and failure handler inside the open_new_stream_and_recycle_message * Modify the way we return results * Rename function name and variable * Changed an error message * Appeased clippy * Appeased formatter * master-commented: Corrected TODO comment * GH-677: add the comment over stream_handler_pool, to make it easier to differentiate from the other * GH-677: revert the rename to StreamHandlerPool * GH-677: revert the rename to StreamHandlerPoolSubs * GH-677: revert the rename to StreamHandlerPoolCluster * GH-677: revert the name from NeighborPublicKey to Key inside the enum Endpoint * GH-677: rename the filename from neighbor_stream_handler_pool.rs to stream_handler_pool.rs * GH-677: rename NeighborStreamHandlerPool to StreamHandlerPool at other places * GH-677: revert rename in the reamining places * GH-677: rename in crash_command.rs * GH-677: change the debug to an error * GH-677: remove the comment 'way to big' * GH-678: initialize ScheduleMessage * GH-678: send a message succusfully * GH-678: rename the name to MessageScheduler * GH-678: introduce the schedule_msg as a field * GH-678: reorder imports * GH-678: fix the Send build error with schedule msg * GH-678: use the generic Message Type for the MessageScheduler * GH-679: add the ability to send the schedule msg * GH-678: integrate the message scheduler with the old code * GH-678: remove unnecessary bounds from the MessageScheduler * GH-678: code cleanup * GH-678: remove the NotifyHandler field from StreamHandlerPool * GH-678: make the handler for the MessageScheduler generic * GH-678: use ctx for notify later instead of NotifyHandlerReal * GH-678: formatting fixes * GH-678: migrate MessageScheduler to the messages.rs * GH-678: Review 1 Changes (#253) * feat: migrate MessageScheduler to the sub_lib/utils.rs * feat: rename to scheduled_msg * GH-678: rename duration to delay * GH-678: rename the name of the sub * GH-678: add a todo for the procedural macro card * GH-678: rename to scheduled_node_query_response_sub * GH-678: migrate the MessageScheduler handler for Recorder after the recorder_message_handler implementaions --------- Co-authored-by: Dan Wiebe <[email protected]> * fix: replaced old copyright in scripts with masq copyright (#240) * fix: replaced sub copyright with masq copyright * fix: updated copyright to single year instead of range --------- Co-authored-by: Bert <[email protected]> * GH-688: HOTFIX: Remove version incompatibility issue that's occurring on v0.7.2 (#259) * GH-688: rename the field originator_alias_public_key to originator_public_key * GH-688: renames in multinode test * GH-688: increase the hard limit for bookkeeping test from 3.5s to 7s --------- Co-authored-by: KauriHero <[email protected]> * GH-671: base for PaymentAdjuster-checked payables (#233) * GH-671: started with the reconstruction slowly; however, found weakness in an older test and designed a test tool, eventually bracing it up * GH-671: I stopped before I create another branch to fix some discrepancy in the test tree (adding tests + I wanna move my new test utility there too) * testing_area_fixes: one test counted on a premise that could not be matched with the test tooling we had; I created a new kind and fixed the test * GH-673: making three tests into more stronger ones; these will embrace also certain use of self.handle_scan() * GH-673: first phase of smooth use of stop conditions for recorder * GH-673: first moment hitting wall hard * GH-673: StopConditions considered implemented; might change some details * GH-673: added some tests... of test code... * GH-673: finishing tests and adding required PartialEq and Eq traits to some messages * GH-673: most of things from the review addressed; preparing for reducing the Single cond, experimenting * GH-673: review answered * GH-673: adding notify later 'facelift' * GH-673: simplifying default impl for NotifyLaterHandler * GH-671: merge from GH-673 finished * GH-671: interim commit * GH-671: blockchain bridge with implemented check for balances for payables * GH-671: enclosed the circle; functionality stable again; fully tested; next some reviewing * GH-671: last changes begun with auto review * GH-671: formatting * GH-671: removing www.failingFailing.com as non-http anymore for www.neverssl.com * GH-671: formatting * GH-671: first part of addressing the review * GH-671: problematic parts that required discussion resolved now * GH-671: refactoring my older code in setup_reporter.rs so that it makes more sense * GH-671: weis of gwei constant renamed * GH-671: making function clearer by using better names for variables etc * GH-671: added a comment to explain more about the fn * GH-671: little mistake in multinode tests --------- Co-authored-by: Bert <[email protected]> * GH-675: Find Alternative to route -n in Linux (#251) --------- Co-authored-by: Syther007 <[email protected]> Co-authored-by: Utkarsh Gupta <[email protected]> Co-authored-by: Dan Wiebe <[email protected]> Co-authored-by: masqrauder <[email protected]> Co-authored-by: Bert <[email protected]> Co-authored-by: Bert <[email protected]> Co-authored-by: Czarte <[email protected]> Co-authored-by: Vojtěch Parkán <[email protected]> Co-authored-by: KauriHero <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.