move stop signal listen to run function and refactor launch batcher by MayRosenbaum · Pull Request #648 · hyperledger/fabric-x-orderer

MayRosenbaum · 2026-02-25T14:31:10Z

StopSignalListen method is currently running as part of the launch method.
To be able to close the inner go routine in that method and call it again during dynamic reconfig, a refactor is needed.
The launchBatcher method is also refactored to be similar to the launchRouter method.

Relevant issue: #504

Copilot

Pull request overview

This PR refactors node startup/shutdown wiring to support re-invoking SIGTERM listeners during lifecycle changes (e.g., dynamic reconfig), and aligns batcher launch flow with the router’s “start service then run” pattern.

Changes:

Moved SIGTERM handling into common/utils as StopSignalListen(stopChan, ...) and wired it into Batcher.Run().
Refactored batcher launch to start its gRPC service via Batcher.StartBatcherService() and adjusted CreateBatcher to no longer require a Net at construction time.
Updated tests and router service setup to match the new batcher initialization flow.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`common/utils/net.go`	Introduces reusable `StopSignalListen` with optional lifecycle stop channel.
`node/batcher/batcher.go`	Adds `StartBatcherService()`, `Address()`, and installs SIGTERM listener from within `Run()`.
`node/batcher/batcher_builder.go`	Changes `CreateBatcher` signature to no longer accept `Net`.
`node/server/arma.go`	Updates batcher launch path to use `StartBatcherService()`; switches SIGTERM wiring to `utils.StopSignalListen`.
`node/router/router.go`	Sets `r.net` earlier in `StartRouterService()`.
`node/batcher/utils_test.go`	Updates batcher creation to set `Net` after construction.
`test/utils_test.go`	Updates batcher creation to set `Net` after construction.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

node/server/arma.go

Copilot · 2026-02-25T15:37:06Z

node/batcher/batcher.go

 	b.logger.Infof("Starting batcher")
 	b.batcher.Start()
 	b.metrics.Start()
+	utils.StopSignalListen(b.stopChan, b, b.logger, b.Address())
 }


CreateBatcher no longer wires Net, but Run() now installs StopSignalListen (which will call b.Stop() on SIGTERM) and Stop() unconditionally calls b.Net.Stop(). To keep the API hard-to-misuse, consider enforcing b.Net is set before Run() (panic/return error with a clear message) or changing construction so Net is always initialized (e.g., StartBatcherService inside Run, or a constructor option).

This follows the launch router design

b.StartBatcherService() is responsible for initializing a gRPC server.

node/server/arma.go

Copilot · 2026-02-25T15:37:07Z

common/utils/net.go

+func StopSignalListen(stopChan chan struct{}, node NodeStopper, logger *flogging.FabricLogger, nodeAddr string) {
+	signalChan := make(chan os.Signal, 1)
+	signal.Notify(signalChan, syscall.SIGTERM)
+
+	go func() {
+		defer signal.Stop(signalChan)
+
+		for {
+			select {
+			case <-signalChan:
+				logger.Infof("SIGTERM signal caught, the node listening on %s is about to shutdown:", nodeAddr)
+				node.Stop()
+				return
+			case <-stopChan:
+				logger.Infof("Exit StopSignalListen routine")
+				return
+			}
+		}
+	}()


StopSignalListen is newly added to net.go, but there are already unit tests for this file (common/utils/net_test.go) and the new shutdown behavior isn’t covered. Adding a test that closes stopChan and asserts the listener exits (and does not call node.Stop()) would help prevent goroutine leaks/regressions.

Lets add an issue to unit test it after the dust settles on the main/launch refactoring.

node/server/arma.go

HagarMeir · 2026-02-25T18:54:11Z

node/batcher/batcher_builder.go

 	"github.com/hyperledger/fabric-x-orderer/request"
 )

-func CreateBatcher(config *node_config.BatcherNodeConfig, logger *flogging.FabricLogger, net Net, cdrc ConsensusDecisionReplicatorCreator, senderCreator ConsenterControlEventSenderCreator, signer Signer) *Batcher {


why do we need to remove net from the batcher?
and now always remember to set it from the outside (otherwise it will panic on batcher stop)

tock-ibm · 2026-03-02T08:25:42Z

common/utils/net.go

I would put this under node/server in a separate file "signal_listener.go".

tock-ibm · 2026-03-02T08:26:24Z

common/utils/net.go

+func StopSignalListen(stopChan chan struct{}, node NodeStopper, logger *flogging.FabricLogger, nodeAddr string) {
+	signalChan := make(chan os.Signal, 1)
+	signal.Notify(signalChan, syscall.SIGTERM)
+
+	go func() {
+		defer signal.Stop(signalChan)
+
+		for {
+			select {
+			case <-signalChan:
+				logger.Infof("SIGTERM signal caught, the node listening on %s is about to shutdown:", nodeAddr)
+				node.Stop()
+				return
+			case <-stopChan:
+				logger.Infof("Exit StopSignalListen routine")
+				return
+			}
+		}
+	}()


Lets add an issue to unit test it after the dust settles on the main/launch refactoring.

tock-ibm · 2026-03-02T08:28:59Z

node/batcher/batcher.go

+	go func() {
+		err := srv.Start()
+		if err != nil {
+			panic(err)


Is every time the server exits with an error deserves a panic?

That is what happens in the router.
In the other nodes, we ignore the error

Signed-off-by: May.Buzaglo <May.Buzaglo@ibm.com>

DorKatzelnick requested a review from Copilot February 25, 2026 15:30

Copilot started reviewing on behalf of DorKatzelnick February 25, 2026 15:31 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

HagarMeir reviewed Feb 25, 2026

View reviewed changes

tock-ibm force-pushed the move_stop_signal_listen_to_run_function branch from 5d84ed2 to b40df83 Compare March 2, 2026 07:03

tock-ibm reviewed Mar 2, 2026

View reviewed changes

MayRosenbaum mentioned this pull request Mar 2, 2026

unit test StopSignalListen #656

Open

MayRosenbaum force-pushed the move_stop_signal_listen_to_run_function branch from b40df83 to 07a4c95 Compare March 2, 2026 09:12

MayRosenbaum added 3 commits March 2, 2026 11:29

move stop signal listen to run function and refactor launch batcher

c48d7d1

Signed-off-by: May.Buzaglo <May.Buzaglo@ibm.com>

add comment

ccc7fb3

Signed-off-by: May.Buzaglo <May.Buzaglo@ibm.com>

address review comments and make node utils pkg

6c243b5

Signed-off-by: May.Buzaglo <May.Buzaglo@ibm.com>

tock-ibm force-pushed the move_stop_signal_listen_to_run_function branch from 07a4c95 to 6c243b5 Compare March 2, 2026 09:29

tock-ibm approved these changes Mar 2, 2026

View reviewed changes

MayRosenbaum merged commit 02e0908 into hyperledger:main Mar 2, 2026
9 of 10 checks passed

Conversation

MayRosenbaum commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MayRosenbaum commented Feb 25, 2026 •

edited

Loading