Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix localhost monitor return #33

Merged
merged 8 commits into from
Oct 14, 2015
Merged

Conversation

ineiti
Copy link
Member

@ineiti ineiti commented Oct 9, 2015

OK, this is the correct fix for #31. I added a new method to the platform.Platform interface, Wait which will wait for any started processes to quit. Start is non-blocking, so we can start an eventual monitor.

There were some issues with proto/sign with regard to correctly quit the signing. Now there is a CloseAll message in the handling of the messages which starts from the root-node and propagates to all children nodes.

@ineiti
Copy link
Member Author

ineiti commented Oct 9, 2015

Closes #31, Closes #24, Closes #25

@@ -190,6 +207,9 @@ func (d *Localhost) Stop() error {
if err != nil {
dbg.Lvl3("Error stopping localhost", err)
}

// Wait for eventual connections to clean up
time.Sleep(time.Second)
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for both Wait() + Stop() since basically they do the same thing, one is blocking whereas the other is not.
We could somehow use a Wait() for regular "stops" of the platform and a Clean / Cleanup () for killing everything the hardway (the way Stop() is done right now), when a process can not finish after a certain timeout.
Another possiblity is to transform Wait() to take a timeout. If the timeout is passed, then Wait() kills the processes and return back to the caller. That way, both functionalities are present and there is no one more method to the interface.

@ineiti
Copy link
Member Author

ineiti commented Oct 12, 2015

Looks good to me - comments?

@nikkolasg
Copy link
Contributor

Looks fine for me. There are still some issues regarding the "if platform == "deterlab" " but that kind of thing will have to be resolved in the future and is not the main concern here.
it's been two days. Shall we merge ?

@bford
Copy link

bford commented Oct 14, 2015

Sounds good. (And for platform/testbed-related stuff don't feel like you need to get my sign-off before merging; just the two of you [Linus & Nicolas])

nikkolasg added a commit that referenced this pull request Oct 14, 2015
Fix localhost monitor return. Merge reviewed and accepted.
@nikkolasg nikkolasg merged commit cc1974b into development Oct 14, 2015
@ineiti ineiti deleted the fix_localhost_monitor_return branch October 15, 2015 14:13
jeffallen pushed a commit that referenced this pull request Jun 6, 2018
This PR allows verification of darcs when the signature path is empty,
but there exists a digest of the signature path. The server, given an
appropriate callbacks to find darcs, is able to find the missing path
and use that for the verification.  This is only the first step, more
improvements are planned in the future, which we describe in #33.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants