Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Additional logging and friendlier error messages #893

Merged
merged 10 commits into from
Apr 7, 2016
Merged

Additional logging and friendlier error messages #893

merged 10 commits into from
Apr 7, 2016

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Apr 6, 2016

Probably solves #848 also - when compiled in debug it takes significant amount of time between Closing... message and stopping the application (my cpu goes high during that time also) - I will check backtraces what's happening in that particular moment, but having hard time to reproduce stalling after Ctrl+C.

@tomusdrw tomusdrw added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Apr 6, 2016
@arkpar
Copy link
Collaborator

arkpar commented Apr 6, 2016

The delay is due to importing 128 blocks at a time in client.rs There is no early exit in this loop I believe

@@ -231,8 +231,8 @@ impl<Message> Handler for IoManager<Message> where Message: Send + Clone + Sync
fn notify(&mut self, event_loop: &mut EventLoop<Self>, msg: Self::Message) {
match msg {
IoMessage::Shutdown => {
self.workers.clear();
event_loop.shutdown();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arkpar Could you have a look at this? Does it make any sense to shutdown event_loop before clearing workers?

clear() drops workers and locks until they process all work, but my guess is that some messages might still be added to the channel for them if we don't shutdown event_loop?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If event loop is destroyed while workers are active they might panic trying to send notification to the event loop. Any particular reason to remove this?
If there are unhandled events left in the event loop that's fine cause we just ignore them. We don't have to process anything after Shutdown event.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, reverting. I thought that because event_loop is not shut down workers may still get additional work because event_loop is active. My understanding was that swapping those two lines will block any new events from event_loop and after that we will wait for workers to process what's pending.

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Apr 7, 2016
@arkpar arkpar added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 7, 2016
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Apr 7, 2016
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 7, 2016
@arkpar arkpar merged commit 123a0f0 into master Apr 7, 2016
@tomusdrw tomusdrw deleted the closing branch April 9, 2016 09:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants