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

Launch http bridge and wallet together #38

Merged
merged 5 commits into from
Mar 15, 2019
Merged

Launch http bridge and wallet together #38

merged 5 commits into from
Mar 15, 2019

Conversation

Anviking
Copy link
Collaborator

@Anviking Anviking commented Mar 7, 2019

Issue Number

#8

Overview

  • I have created a new CLI module for logic to be shared between multiple executables
  • I have added a new cardano-wallet-launcher executable
  • WIP Launcher tests (I'm seeing some strange things)

Comments

  • I have now rebased into 3 commits. View by commit might be useful

  • The wallet crashes when it's trying to connect to the node that is still launching 😞 . If we only need 5 lines of retry mechanism it should be simple though.

  • While I see strange things in unit tests / ghci, I also believe this is working. (from testing with stack exec)

@Anviking Anviking force-pushed the anviking/8/launcher branch 2 times, most recently from 4423ec3 to b6928ea Compare March 7, 2019 17:21
@Anviking Anviking self-assigned this Mar 7, 2019
bracketProcess :: CreateProcess -> IO () -> IO ()
bracketProcess process action = do
_ <- withCreateProcess
process {delegate_ctlc = True, std_out = CreatePipe }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With this delegate_ctlc, ctrl-d works. ctrl-c still doesn't work…

I'd also like to redirect std_out, but don't know how yet.

app/server/Launcher.hs Outdated Show resolved Hide resolved
app/server/Launcher.hs Outdated Show resolved Hide resolved
app/server/Launcher.hs Outdated Show resolved Hide resolved
app/server/Launcher.hs Outdated Show resolved Hide resolved
paweljakubas added a commit that referenced this pull request Mar 11, 2019
[3] Adding ticking function test and downloading block logic

[3] Block syncer working ok plus additional tests

[3] Cleaning the code

[3] cabal fix

[3] Complete refactoring to the review

[3] Final refactoring

[3] cabal fix

[3] hlint and weeding

[3] add killing thread at the end of test

[3] Replace IORef with MVar

refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints

refactoring #2 | review indentation of 'where' clause

refactoring #3 | inline loop and remove test intrumentation from test logic

refactoring #4 | define generator for ticking args in a declarative manner

refactoring #5 | purify tickingFunctionTest and make it a monadic property

refactoring #6 | review naming in Arbitrary TickingArgs

refactoring #7 | use guards in mkConsecutiveTestBlocks

refactoring #8 | define single block generator from previous block

refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list

refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function

refactoring #11 | switch argument positions in mkConsecutiveTestBlocks

refactoring #12 | replace loop with built-in list 'iterate'

refactoring #13 | define Arbitrary instance for creating consecutive blocks

refactoring #14 | replace mkConsecutiveBlocks with a property parameter

refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar'

refactoring #16 | remove unecessary IO in 'writeToIORefAction'

refactoring #17 | replace takeMVar + putMVar with modifyMVar

refactoring #18 | review naming for 'writeToIORefAction' --> 'reader'

refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match

refactoring #20 | remove 'BlocksConsumed' wrapper

refactoring #21 | generalize reader with polymorphic parametrism

refactoring #22 | review pushNextBlocks indentation

refactoring #23 | group case pattern matches using tuple

refactoring #24 | remove 'Hash BlockHeader' from the block to inject

refactoring #25 | use synchronization lock instead of computed times

refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks'

refactoring #27 | remove 'chunkSizes' in a favor of inline random selection

refactoring #28 | remove 'DeliveryMode' in favor of the most general case

refactoring #29 | cleanup wrapper types

refactoring #30 | generalize pushNextBlocks with parametric polymorphism

refactoring #31 | rename pushNextBlocks to 'writer'

refactoring #32 | define reader on Block instead of BlockHeaderHash

refactoring #33 | replace old reader with reader'

refactoring #34 | move creation of writer MVar inside writer action

refactoring #35 | remove header hash from 'Blocks'

refactoring #36 | rename 'consecutiveBlocks' into 'blocks'

refactoring #37 | re-organize module to separate effectful logic from declarations

refactoring #38 | move waiting logic into dedicated function

refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer

refactoring #40 | use Millisecond instead of Second for shorter tests

refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks'

[3] fix line width

[3] aligning the code with other code changes
paweljakubas added a commit that referenced this pull request Mar 11, 2019
[3] Adding ticking function test and downloading block logic

[3] Block syncer working ok plus additional tests

[3] Cleaning the code

[3] cabal fix

[3] Complete refactoring to the review

[3] Final refactoring

[3] cabal fix

[3] hlint and weeding

[3] add killing thread at the end of test

[3] Replace IORef with MVar

refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints

refactoring #2 | review indentation of 'where' clause

refactoring #3 | inline loop and remove test intrumentation from test logic

refactoring #4 | define generator for ticking args in a declarative manner

refactoring #5 | purify tickingFunctionTest and make it a monadic property

refactoring #6 | review naming in Arbitrary TickingArgs

refactoring #7 | use guards in mkConsecutiveTestBlocks

refactoring #8 | define single block generator from previous block

refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list

refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function

refactoring #11 | switch argument positions in mkConsecutiveTestBlocks

refactoring #12 | replace loop with built-in list 'iterate'

refactoring #13 | define Arbitrary instance for creating consecutive blocks

refactoring #14 | replace mkConsecutiveBlocks with a property parameter

refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar'

refactoring #16 | remove unecessary IO in 'writeToIORefAction'

refactoring #17 | replace takeMVar + putMVar with modifyMVar

refactoring #18 | review naming for 'writeToIORefAction' --> 'reader'

refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match

refactoring #20 | remove 'BlocksConsumed' wrapper

refactoring #21 | generalize reader with polymorphic parametrism

refactoring #22 | review pushNextBlocks indentation

refactoring #23 | group case pattern matches using tuple

refactoring #24 | remove 'Hash BlockHeader' from the block to inject

refactoring #25 | use synchronization lock instead of computed times

refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks'

refactoring #27 | remove 'chunkSizes' in a favor of inline random selection

refactoring #28 | remove 'DeliveryMode' in favor of the most general case

refactoring #29 | cleanup wrapper types

refactoring #30 | generalize pushNextBlocks with parametric polymorphism

refactoring #31 | rename pushNextBlocks to 'writer'

refactoring #32 | define reader on Block instead of BlockHeaderHash

refactoring #33 | replace old reader with reader'

refactoring #34 | move creation of writer MVar inside writer action

refactoring #35 | remove header hash from 'Blocks'

refactoring #36 | rename 'consecutiveBlocks' into 'blocks'

refactoring #37 | re-organize module to separate effectful logic from declarations

refactoring #38 | move waiting logic into dedicated function

refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer

refactoring #40 | use Millisecond instead of Second for shorter tests

refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks'

[3] fix line width

[3] aligning the code with other code changes

[3] hlint suggestion
paweljakubas added a commit that referenced this pull request Mar 11, 2019
[3] Adding ticking function test and downloading block logic

[3] Block syncer working ok plus additional tests

[3] Cleaning the code

[3] cabal fix

[3] Complete refactoring to the review

[3] Final refactoring

[3] cabal fix

[3] hlint and weeding

[3] add killing thread at the end of test

[3] Replace IORef with MVar

refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints

refactoring #2 | review indentation of 'where' clause

refactoring #3 | inline loop and remove test intrumentation from test logic

refactoring #4 | define generator for ticking args in a declarative manner

refactoring #5 | purify tickingFunctionTest and make it a monadic property

refactoring #6 | review naming in Arbitrary TickingArgs

refactoring #7 | use guards in mkConsecutiveTestBlocks

refactoring #8 | define single block generator from previous block

refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list

refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function

refactoring #11 | switch argument positions in mkConsecutiveTestBlocks

refactoring #12 | replace loop with built-in list 'iterate'

refactoring #13 | define Arbitrary instance for creating consecutive blocks

refactoring #14 | replace mkConsecutiveBlocks with a property parameter

refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar'

refactoring #16 | remove unecessary IO in 'writeToIORefAction'

refactoring #17 | replace takeMVar + putMVar with modifyMVar

refactoring #18 | review naming for 'writeToIORefAction' --> 'reader'

refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match

refactoring #20 | remove 'BlocksConsumed' wrapper

refactoring #21 | generalize reader with polymorphic parametrism

refactoring #22 | review pushNextBlocks indentation

refactoring #23 | group case pattern matches using tuple

refactoring #24 | remove 'Hash BlockHeader' from the block to inject

refactoring #25 | use synchronization lock instead of computed times

refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks'

refactoring #27 | remove 'chunkSizes' in a favor of inline random selection

refactoring #28 | remove 'DeliveryMode' in favor of the most general case

refactoring #29 | cleanup wrapper types

refactoring #30 | generalize pushNextBlocks with parametric polymorphism

refactoring #31 | rename pushNextBlocks to 'writer'

refactoring #32 | define reader on Block instead of BlockHeaderHash

refactoring #33 | replace old reader with reader'

refactoring #34 | move creation of writer MVar inside writer action

refactoring #35 | remove header hash from 'Blocks'

refactoring #36 | rename 'consecutiveBlocks' into 'blocks'

refactoring #37 | re-organize module to separate effectful logic from declarations

refactoring #38 | move waiting logic into dedicated function

refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer

refactoring #40 | use Millisecond instead of Second for shorter tests

refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks'

[3] fix line width

[3] aligning the code with other code changes

[3] hlint suggestion

[3] add .weeder.yaml to omit duplicateMaybes and groups to be announce during weeder execution

[3] undo weeder ignore file plus remove pragma and unnecessary exports
@Anviking Anviking changed the title Launch http bridge and wallet together [WIP] Launch http bridge and wallet together Mar 11, 2019
paweljakubas added a commit that referenced this pull request Mar 11, 2019
[3] Adding ticking function test and downloading block logic

[3] Block syncer working ok plus additional tests

[3] Cleaning the code

[3] cabal fix

[3] Complete refactoring to the review

[3] Final refactoring

[3] cabal fix

[3] hlint and weeding

[3] add killing thread at the end of test

[3] Replace IORef with MVar

refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints

refactoring #2 | review indentation of 'where' clause

refactoring #3 | inline loop and remove test intrumentation from test logic

refactoring #4 | define generator for ticking args in a declarative manner

refactoring #5 | purify tickingFunctionTest and make it a monadic property

refactoring #6 | review naming in Arbitrary TickingArgs

refactoring #7 | use guards in mkConsecutiveTestBlocks

refactoring #8 | define single block generator from previous block

refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list

refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function

refactoring #11 | switch argument positions in mkConsecutiveTestBlocks

refactoring #12 | replace loop with built-in list 'iterate'

refactoring #13 | define Arbitrary instance for creating consecutive blocks

refactoring #14 | replace mkConsecutiveBlocks with a property parameter

refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar'

refactoring #16 | remove unecessary IO in 'writeToIORefAction'

refactoring #17 | replace takeMVar + putMVar with modifyMVar

refactoring #18 | review naming for 'writeToIORefAction' --> 'reader'

refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match

refactoring #20 | remove 'BlocksConsumed' wrapper

refactoring #21 | generalize reader with polymorphic parametrism

refactoring #22 | review pushNextBlocks indentation

refactoring #23 | group case pattern matches using tuple

refactoring #24 | remove 'Hash BlockHeader' from the block to inject

refactoring #25 | use synchronization lock instead of computed times

refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks'

refactoring #27 | remove 'chunkSizes' in a favor of inline random selection

refactoring #28 | remove 'DeliveryMode' in favor of the most general case

refactoring #29 | cleanup wrapper types

refactoring #30 | generalize pushNextBlocks with parametric polymorphism

refactoring #31 | rename pushNextBlocks to 'writer'

refactoring #32 | define reader on Block instead of BlockHeaderHash

refactoring #33 | replace old reader with reader'

refactoring #34 | move creation of writer MVar inside writer action

refactoring #35 | remove header hash from 'Blocks'

refactoring #36 | rename 'consecutiveBlocks' into 'blocks'

refactoring #37 | re-organize module to separate effectful logic from declarations

refactoring #38 | move waiting logic into dedicated function

refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer

refactoring #40 | use Millisecond instead of Second for shorter tests

refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks'

[3] fix line width

[3] aligning the code with other code changes

[3] hlint suggestion

[3] add .weeder.yaml to omit duplicateMaybes and groups to be announce during weeder execution

[3] undo weeder ignore file plus remove pragma and unnecessary exports

[3] remove not needed imports
@Anviking Anviking force-pushed the anviking/8/launcher branch 3 times, most recently from a6830ff to 961c9bb Compare March 11, 2019 16:37
|]


type Process = (Maybe Handle, Maybe Handle, Maybe Handle, ProcessHandle)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did this for cleanupProcess. Not sure if it is needed.

Copy link
Member

Choose a reason for hiding this comment

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

What you probably really want here is a record :)

app/launcher/Main.hs Outdated Show resolved Hide resolved

#while true; do echo "I'm a wallet"; sleep 1; done
echo "I'm a wallet"
sleep 20
Copy link
Collaborator Author

@Anviking Anviking Mar 11, 2019

Choose a reason for hiding this comment

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

Idea was to (at least) have temporary manual tests using files like this 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Oui 👍

@Anviking Anviking requested a review from KtorZ March 12, 2019 08:46
app/cli/CLI.hs Outdated Show resolved Hide resolved
|]


type Process = (Maybe Handle, Maybe Handle, Maybe Handle, ProcessHandle)
Copy link
Member

Choose a reason for hiding this comment

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

What you probably really want here is a record :)

app/launcher/Main.hs Outdated Show resolved Hide resolved
app/launcher/Main.hs Outdated Show resolved Hide resolved
app/launcher/Main.hs Outdated Show resolved Hide resolved
interruptProcessGroupOf h
cleanupProcess p

nodeHttpBridgeOn :: Port "Node" -> Network -> CreateProcess
Copy link
Member

Choose a reason for hiding this comment

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

Port (* :: Symbol) --> Nice one :)


#while true; do echo "I'm a wallet"; sleep 1; done
echo "I'm a wallet"
sleep 20
Copy link
Member

Choose a reason for hiding this comment

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

Oui 👍

app/launcher/wallet.sh Outdated Show resolved Hide resolved
@Anviking Anviking changed the title [WIP] Launch http bridge and wallet together Launch http bridge and wallet together Mar 14, 2019
@Anviking Anviking force-pushed the anviking/8/launcher branch 4 times, most recently from 877b2c7 to 93e32c4 Compare March 14, 2019 22:24
@Anviking Anviking requested review from KtorZ and rvl March 14, 2019 23:21
@rvl
Copy link
Contributor

rvl commented Mar 15, 2019

If your tests are failing, it could be because the tests are correct , and there is something missing from supervise.

I pushed a commit with an example of using withCreateProcess and race_ together. Then it's all done by the libraries and you don't need the supervise code.

What's the purpose of having a separate cardano-wallet-launcher executable? Unless I'm missing something, it can all be done from within cardano-wallet-server. We would use async to manage concurrent wallet tasks and a single subprocess for the node backend.

The wallet crashes when it's trying to connect to the node that is still launching
Yes, good catch. Sleeping for 1 second is one simple way of dealing with that.

@@ -83,9 +83,8 @@ executable cardano-wallet-server
NoImplicitPrelude
OverloadedStrings
ghc-options:
-threaded -rtsopts
Copy link
Member

Choose a reason for hiding this comment

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

We gotta need this 😅 ... If the wallet server isn't threaded, it isn't going to do much serving requests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops 😅 was supposed to be temporary

@KtorZ KtorZ force-pushed the anviking/8/launcher branch 2 times, most recently from fc6bde1 to fb3ed7e Compare March 15, 2019 13:58
Anviking and others added 5 commits March 15, 2019 14:58
To be shared between all executables (cardano-wallet-server and
cardano-wallet-launcher), and by providing an @encodable@ class, should
make it easy for the launcher to forward arguments to the server.

(launcher is introduced in a later commit)
It will ensure that:
- the subprocess is killed if the main thread dies.
- if the subprocess exits, the main thread will be cancelled.
- allow launcher's command to take a 'setup' action before they're ran
  This allows for the wallet backend to wait a bit before it starts. Or,
  any kind of startup action we may think of, like generating TLS certs,
  etc ...

- review folder organization and tests scripts

- add Buildable intance to 'Command' and use that to format the initial info

- use --http-bridge-port instead of --node-port
@KtorZ KtorZ merged commit 8026e8c into master Mar 15, 2019
@KtorZ KtorZ deleted the anviking/8/launcher branch March 15, 2019 14:28
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.

None yet

3 participants