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

Start IPC channel as soon as the wallet BE socket is available #1009

Merged
merged 1 commit into from
Nov 12, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Nov 12, 2019

Issue Number

Overview

  • I have revised the starting order and "dependencies" to make sure that the DaedalusIPC server is started as soon as we know the wallet server port, and before we try to setup the networking layer.

  • reviewed the starting logic with Daedalus team who confirmed that they have error-handling mechanism in place to actually repeatedly poll the API server after a "Started" message is received via IPC. This is necessary since we'll now be starting the IPC channel much before the API is actually ready.

  • Since we have disabled the IPC tests in CI, I've made sure they still pass locally.

Comments

And do not wait for Jörmungandr to be available.
@KtorZ KtorZ requested a review from rvl November 12, 2019 13:00
@KtorZ KtorZ self-assigned this Nov 12, 2019
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Neat 👌

@KtorZ
Copy link
Member Author

KtorZ commented Nov 12, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request Nov 12, 2019
1009: Start IPC channel as soon as the wallet BE socket is available r=KtorZ a=KtorZ



# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have revised the starting order and "dependencies" to make sure that the DaedalusIPC server is started as soon as we know the wallet server port, and before we try to setup the networking layer. 

- [x] reviewed the starting logic with Daedalus team who confirmed that they have error-handling mechanism in place to actually repeatedly poll the API server after a "Started" message is received via IPC. This is necessary since we'll now be starting the IPC channel much before the API is actually ready. 

- [x] Since we have disabled the IPC tests in CI, I've made sure they still pass locally.

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
seqApi <- apiLayer tr seqTl nl
startServer tr nPort bp rndApi seqApi spl
Left e -> handleNetworkStartupError e
Server.withListeningSocket hostPref listen $ \case
Copy link
Contributor

Choose a reason for hiding this comment

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

With this, I think the listening socket will queue connections for API requests, but they will only be accepted once the api server has started.

I do not know what the default listen queue length is.

It may cause problems if Daedalus is polling our API waiting for it to accept connections, depending on how the polling is done.

Maybe a future improvement will be to send ReplyPort only once the API server is ready to accept connections

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe a future improvement will be to send ReplyPort only once the API server is ready to accept connections

Could be 🤔 I know that the Daedalus setup works a bit like a state-machine. I think that, moving forward (mainnet, and likely, the next snapshot release as well), we'll need to review this state machine together and see exactly what's doable and how to better approach this problem.

Choose a reason for hiding this comment

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

@rvl @KtorZ Daedalus only polls network-information endpoint to determine if the API is responding.

Choose a reason for hiding this comment

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

Only after we get a response from that very endpoint we start triggering other API requests...

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 12, 2019

Build succeeded

@iohk-bors iohk-bors bot merged commit f8cf1ff into master Nov 12, 2019
@KtorZ KtorZ deleted the KtorZ/703/daedalus-ipc-earlier-feedback branch November 12, 2019 19:19
@KtorZ KtorZ added this to the Usability & Compatibility milestone Nov 12, 2019
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