-
Notifications
You must be signed in to change notification settings - Fork 0
Review fixes #13
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
base: ipc-client
Are you sure you want to change the base?
Review fixes #13
Changes from all commits
0ee6a3a
6f0d596
924a01a
f4156d3
0bc2ed4
7cc4c1b
1718933
4556098
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,6 +105,11 @@ func (n *LibP2PNetwork) Send(ctx context.Context, msg any, receivers ...peer.ID) | |
| func (n *LibP2PNetwork) SendMsgs(ctx context.Context, messages MsgQueue, receiver peer.ID) (resErr error) { | ||
| var stream libp2pNetwork.Stream | ||
| var err error | ||
| defer func() { | ||
| if stream != nil { | ||
| stream.Close() | ||
| } | ||
| }() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue #2 mentioned a second bug alongside the stream leak: |
||
| for messages.Len() > 0 { | ||
| msg := messages.PopFront() | ||
| // create a stream with first message | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,49 +128,78 @@ func (n *Node) handleLedgerReplicationRequest(ctx context.Context, lr *replicati | |
| return n.sendLedgerReplicationResponse(ctx, resp, lr.NodeID) | ||
| } | ||
| n.log.DebugContext(ctx, fmt.Sprintf("Preparing replication response from block %d", startBlock)) | ||
| go func() { | ||
| blocks := make([]*types.Block, 0) | ||
| blockCnt := uint64(0) | ||
| dbIt := n.blockStore.Find(util.Uint64ToBytes(startBlock)) | ||
| defer func() { | ||
| if err := dbIt.Close(); err != nil { | ||
| n.log.WarnContext(ctx, "closing DB iterator", logger.Error(err)) | ||
| } | ||
| }() | ||
| var firstFetchedBlockNumber uint64 | ||
| var lastFetchedBlockNumber uint64 | ||
| var lastFetchedBlock *types.Block | ||
| for ; dbIt.Valid(); dbIt.Next() { | ||
| var bl types.Block | ||
| roundNo := util.BytesToUint64(dbIt.Key()) | ||
| if err := dbIt.Value(&bl); err != nil { | ||
| n.log.WarnContext(ctx, fmt.Sprintf("Ledger replication reply incomplete, failed to read block %d", roundNo), logger.Error(err)) | ||
| break | ||
| } | ||
| lastFetchedBlock = &bl | ||
| if firstFetchedBlockNumber == 0 { | ||
| firstFetchedBlockNumber = roundNo | ||
| } | ||
| lastFetchedBlockNumber = roundNo | ||
| blocks = append(blocks, lastFetchedBlock) | ||
| blockCnt++ | ||
| if blockCnt >= n.conf.replicationConfig.maxReturnBlocks || | ||
| (roundNo >= lr.EndBlockNumber && lr.EndBlockNumber > 0) { | ||
| break | ||
| } | ||
| } | ||
| select { | ||
| case n.replicationCh <- replicationRequest{ctx: ctx, req: lr}: | ||
| // worker will process | ||
| default: | ||
| resp := &replication.LedgerReplicationResponse{ | ||
| UUID: lr.UUID, | ||
| Status: replication.Ok, | ||
| Blocks: blocks, | ||
| FirstBlockNumber: firstFetchedBlockNumber, | ||
| LastBlockNumber: lastFetchedBlockNumber, | ||
| UUID: lr.UUID, | ||
| Status: replication.Busy, | ||
| Message: "Too many concurrent replication requests", | ||
| } | ||
| return n.sendLedgerReplicationResponse(ctx, resp, lr.NodeID) | ||
| } | ||
|
Comment on lines
+131
to
+141
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| return nil | ||
| } | ||
|
|
||
| func (n *Node) replicationLoop(ctx context.Context) { | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case r := <-n.replicationCh: | ||
| n.processReplicationRequest(r.ctx, r.req) | ||
| } | ||
| if err := n.sendLedgerReplicationResponse(ctx, resp, lr.NodeID); err != nil { | ||
| n.log.WarnContext(ctx, fmt.Sprintf("Problem sending ledger replication response, %s", resp.Pretty()), logger.Error(err)) | ||
| } | ||
| } | ||
|
|
||
| func (n *Node) processReplicationRequest(ctx context.Context, lr *replication.LedgerReplicationRequest) { | ||
| startBlock := lr.BeginBlockNumber | ||
| blocks := make([]*types.Block, 0) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| blockCnt := uint64(0) | ||
| dbIt := n.blockStore.Find(util.Uint64ToBytes(startBlock)) | ||
| defer func() { | ||
| if err := dbIt.Close(); err != nil { | ||
| n.log.WarnContext(ctx, "closing DB iterator", logger.Error(err)) | ||
| } | ||
| }() | ||
| return nil | ||
| var firstFetchedBlockNumber uint64 | ||
| var lastFetchedBlockNumber uint64 | ||
| var lastFetchedBlock *types.Block | ||
| for ; dbIt.Valid(); dbIt.Next() { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| default: | ||
| } | ||
| var bl types.Block | ||
| roundNo := util.BytesToUint64(dbIt.Key()) | ||
| if err := dbIt.Value(&bl); err != nil { | ||
| n.log.WarnContext(ctx, fmt.Sprintf("Ledger replication reply incomplete, failed to read block %d", roundNo), logger.Error(err)) | ||
| break | ||
| } | ||
| lastFetchedBlock = &bl | ||
| if firstFetchedBlockNumber == 0 { | ||
| firstFetchedBlockNumber = roundNo | ||
| } | ||
| lastFetchedBlockNumber = roundNo | ||
| blocks = append(blocks, lastFetchedBlock) | ||
| blockCnt++ | ||
| if blockCnt >= n.conf.replicationConfig.maxReturnBlocks || | ||
| (roundNo >= lr.EndBlockNumber && lr.EndBlockNumber > 0) { | ||
| break | ||
| } | ||
| } | ||
| resp := &replication.LedgerReplicationResponse{ | ||
| UUID: lr.UUID, | ||
| Status: replication.Ok, | ||
| Blocks: blocks, | ||
| FirstBlockNumber: firstFetchedBlockNumber, | ||
| LastBlockNumber: lastFetchedBlockNumber, | ||
| } | ||
| if err := n.sendLedgerReplicationResponse(ctx, resp, lr.NodeID); err != nil { | ||
| n.log.WarnContext(ctx, fmt.Sprintf("Problem sending ledger replication response, %s", resp.Pretty()), logger.Error(err)) | ||
| } | ||
| } | ||
|
|
||
| // handleLedgerReplicationResponse handles ledger replication responses from other partition nodes. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
maxMsgSizeof 16MB might be too restrictive when combined with the defaultmaxReturnBlocksof 1000 in ledger replication. If blocks are larger than ~16KB on average, the resultingLedgerReplicationResponsewill exceed this limit and be rejected by the receiver. Consider either increasing this limit or ensuring that the replication logic can handle splitting large responses.