Update ouroboros-network dependency #588

mrBliss · 2020-02-19T12:21:38Z

Issue

Pushing through the changes made in IntersectMBO/ouroboros-network#1657

checklist

This PR contains all the work required to resolve the linked issue.
This PR results in breaking changes to upstream dependencies.
The work contained has sufficient documentation to describe what it does and how to do it.
The work has sufficient tests and/or testing.
I have committed clear and descriptive commits. Be considerate as somebody else will have to read these.
I have added the appropriate labels to this PR.

mrBliss · 2020-02-19T12:22:35Z

@coot There are some breaking changes in the network code, can you fix these?

edsko · 2020-02-19T17:04:58Z

This will need updating after IntersectMBO/ouroboros-network#1674 is merged (I don't think that should be too bad).

karknu · 2020-02-19T18:37:53Z

@mrBliss your branch builds and runs fine for me. What was it that you needed help with?

mrBliss · 2020-02-20T07:48:00Z

@mrBliss your branch builds and runs fine for me. What was it that you needed help with?

I suppose the main node itself runs fine, but the other tools probably wont, as I left a bunch of undefineds in there. Can you have a look through at the diff and look for undefined and other commented out code?

coot

withIOManager starts IO manager thread. On Unix its noop, on Windows however it start a thread which fulfills asynchronous requests (like the GHC's RTS IO manager thread). It must be started once, when its started multiple times the behavior is undefined; for that reason it must be started when the application starts and the AssociateWithIOCP function can be passed down to call sites when we create Snockets (an abstraction over Berkeley sockets and Windows' named pipes).

coot · 2020-02-20T15:03:29Z

cardano-node/src/Cardano/CLI/Benchmarking/Tx/NodeToNode.hs

@@ -204,15 +206,16 @@ benchmarkConnectTxSubmit
  -> TxSubmissionClient (GenTxId blk) (GenTx blk) m ()
  -- ^ the particular txSubmission peer
  -> m ()
-benchmarkConnectTxSubmit trs nc localAddr remoteAddr myTxSubClient = do
+benchmarkConnectTxSubmit trs nc localAddr remoteAddr myTxSubClient = withIOManager $ \iocp -> do


I think this is wrong place to call withIOManager. It should be started at the very top level when the application starts. We don't want two IO manager threads to complete.

coot · 2020-02-20T15:05:25Z

cardano-node/src/Cardano/Chairman.hs

-      Nothing
-      addr
-    `catch` handleMuxError tracer chainsVar socketPath
+  socketPath = withIOManager $ \iocp -> do


The same here, withIOManager must be called when chairman starts and then iocp :: AssociateWithIOCP should be passed down.

coot · 2020-02-20T15:05:55Z

cardano-node/src/Cardano/Node/Submission.hs

@@ -106,15 +107,15 @@ submitTx :: ( RunNode blk
         -> GenTx blk
         -> Tracer IO TraceLowLevelSubmit
         -> IO ()
-submitTx targetSocketFp protoInfoConfig tx tracer = do
-    targetSocketFp' <- localSocketAddrInfo targetSocketFp
+submitTx targetSocketFp protoInfoConfig tx tracer = NodeToClient.withIOManager $ \iocp -> do


The same applies here.

coot · 2020-02-20T15:06:44Z

cardano-node/src/Cardano/Wallet/Client.hs

@@ -58,16 +59,17 @@ runWalletClient :: forall blk.
                -> SocketPath
                -> Tracer IO String
                -> IO ()
-runWalletClient ptcl sockFp tracer = do
+runWalletClient ptcl sockFp tracer = withIOManager $ \iocp -> do


This is ok, runWalletClient is where the wallet-client starts.

mrBliss · 2020-02-24T10:37:26Z

cardano-node/src/Cardano/CLI/Run.hs

+                                                , withIOManager
+                                                )


@Jimbo4350 What does the style guide say about this? Does the stylish-haskell config in this repo do the right thing?

I don't think it does however I need to update the style guide re: imports. We want this format if imports exceed 100 chars:

import Ouroboros.Network.NodeToClient (AssociateWithIOCP, withIOManager)

Comments were addressed.

mrBliss · 2020-02-25T11:55:20Z

bors r+

588: Update ouroboros-network dependency r=mrBliss a=mrBliss Issue ------ Pushing through the changes made in IntersectMBO/ouroboros-network#1657 checklist --------- - [ ] This PR contains all the work required to resolve the linked issue. - [ ] This PR results in breaking changes to upstream dependencies. - [ ] The work contained has sufficient documentation to describe what it does and how to do it. - [ ] The work has sufficient tests and/or testing. - [ ] I have committed clear and descriptive commits. Be considerate as somebody else will have to read these. - [ ] I have added the appropriate labels to this PR. Co-authored-by: Thomas Winant <thomas@well-typed.com> Co-authored-by: Marcin Szamotulski <profunctor@pm.me>

Jimbo4350 · 2020-02-25T12:31:57Z

bors r-

iohk-bors · 2020-02-25T12:31:59Z

Canceled

cardano-node/src/Cardano/CLI/Run.hs

cardano-node/src/Cardano/CLI/Tx.hs

Jimbo4350

LGTM! Just some conflicts to fix.

After IntersectMBO/ouroboros-network#1657 Co-authored-by: Karl Knutsson <karl.knutsson@iohk.io>

Behaviour of IO manager depends on platform. On Unix its noop, as RTS has an io manager already; on Windows: it starts a thread which fulfills aysynchronous IO request (throuh IO completion port). `withIOManager` is used when an application starts and it must be called only once, so the best place to start is at the very begining of `main`.

intricate

LGTM after rebasing and fixing conflicts.

@Jimbo4350 would you mind giving this a final look?

mrBliss · 2020-02-27T07:31:10Z

I'm going to push another bump of ouroboros-network to this PR. It's already out of date, as it has been open for a week 🙁

EDIT: done

mrBliss · 2020-02-27T08:20:13Z

bors r+

iohk-bors · 2020-02-27T09:10:43Z

Build succeeded

mrBliss requested review from coot and Jimbo4350 February 19, 2020 12:21

mrBliss force-pushed the mrBliss/update-ouroboros-network-20200219 branch 3 times, most recently from ff0b62d to 29f02eb Compare February 19, 2020 13:43

mrBliss force-pushed the mrBliss/update-ouroboros-network-20200219 branch from b23ae48 to 8fb22e6 Compare February 20, 2020 15:00

mrBliss marked this pull request as ready for review February 20, 2020 15:01

coot previously requested changes Feb 20, 2020

View reviewed changes

mrBliss commented Feb 24, 2020

View reviewed changes

mrBliss force-pushed the mrBliss/update-ouroboros-network-20200219 branch from a0e40ff to f19b598 Compare February 25, 2020 08:29

mrBliss requested a review from coot February 25, 2020 09:29

edsko approved these changes Feb 25, 2020

View reviewed changes

Jimbo4350 requested changes Feb 25, 2020

View reviewed changes

cardano-node/src/Cardano/CLI/Run.hs Show resolved Hide resolved

Jimbo4350 reviewed Feb 25, 2020

View reviewed changes

cardano-node/src/Cardano/CLI/Tx.hs Show resolved Hide resolved

Jimbo4350 approved these changes Feb 26, 2020

View reviewed changes

mrBliss and others added 4 commits February 26, 2020 11:32

Update ouroboros-network dependency

4fe75fd

After IntersectMBO/ouroboros-network#1657 Co-authored-by: Karl Knutsson <karl.knutsson@iohk.io>

Add Win32-network dependency

7501e84

call withIOManager within runCommnad

751296f

intricate force-pushed the mrBliss/update-ouroboros-network-20200219 branch from 6bdbf89 to 0346c6d Compare February 26, 2020 17:34

Make changes required after rebase

91cd6ac

intricate force-pushed the mrBliss/update-ouroboros-network-20200219 branch from 0346c6d to 91cd6ac Compare February 26, 2020 17:38

intricate approved these changes Feb 26, 2020

View reviewed changes

Update the dependency on ouroboros-network again

94b440b

iohk-bors bot merged commit b736a1c into master Feb 27, 2020

iohk-bors bot deleted the mrBliss/update-ouroboros-network-20200219 branch February 27, 2020 09:10

mark-stopka mentioned this pull request Oct 11, 2020

[BUG] - Build System: flag -systemd is not being respected by cabal-install #1200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ouroboros-network dependency #588

Update ouroboros-network dependency #588

mrBliss commented Feb 19, 2020

mrBliss commented Feb 19, 2020

edsko commented Feb 19, 2020

karknu commented Feb 19, 2020

mrBliss commented Feb 20, 2020

coot left a comment

coot Feb 20, 2020

coot Feb 20, 2020

coot Feb 20, 2020

coot Feb 20, 2020

mrBliss Feb 24, 2020

Jimbo4350 Feb 25, 2020 •

edited

Loading

mrBliss commented Feb 25, 2020

Jimbo4350 commented Feb 25, 2020

iohk-bors bot commented Feb 25, 2020

Jimbo4350 left a comment •

edited

Loading

intricate left a comment

mrBliss commented Feb 27, 2020 •

edited

Loading

mrBliss commented Feb 27, 2020

iohk-bors bot commented Feb 27, 2020

Update ouroboros-network dependency #588

Update ouroboros-network dependency #588

Conversation

mrBliss commented Feb 19, 2020

Issue

checklist

mrBliss commented Feb 19, 2020

edsko commented Feb 19, 2020

karknu commented Feb 19, 2020

mrBliss commented Feb 20, 2020

coot left a comment

Choose a reason for hiding this comment

coot Feb 20, 2020

Choose a reason for hiding this comment

coot Feb 20, 2020

Choose a reason for hiding this comment

coot Feb 20, 2020

Choose a reason for hiding this comment

coot Feb 20, 2020

Choose a reason for hiding this comment

mrBliss Feb 24, 2020

Choose a reason for hiding this comment

Jimbo4350 Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

mrBliss commented Feb 25, 2020

Jimbo4350 commented Feb 25, 2020

iohk-bors bot commented Feb 25, 2020

Canceled

Jimbo4350 left a comment • edited Loading

Choose a reason for hiding this comment

intricate left a comment

Choose a reason for hiding this comment

mrBliss commented Feb 27, 2020 • edited Loading

mrBliss commented Feb 27, 2020

iohk-bors bot commented Feb 27, 2020

Build succeeded

Jimbo4350 Feb 25, 2020 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading

mrBliss commented Feb 27, 2020 •

edited

Loading