adds Close() method to Input and Output interfaces by rafaelvanoni · Pull Request #19 · farsightsec/go-nmsg

rafaelvanoni · 2026-07-10T17:52:26Z

Once the fix in farsightsec/nmsg#195 was in place, I ran into several small issues in the go-nmsg unit tests. This MR proposes such fixes.

cmikk

This MR fixes a lot of various things. For the purposes of maintaining a useful git history, these fixes should be documented in commit titles and descriptions. I've made a few suggestions of breakdowns, and a few other fixes in comments.

cmikk · 2026-07-14T22:32:30Z

 	co.c.config.Compress = compress
 }
+
+// Close flushes any buffered output and satisfies the interface


This comment should specify "satisfies the Output interface"

cmikk · 2026-07-14T22:32:57Z

 // Read calls the underlying function to return the next message.
 func (i InputFunc) Read() (*Message, error) { return i() }

+// Closes satisfies the interface with a no-op


This comment should specify "satisfies the Input interface with a no-op"

cmikk · 2026-07-14T22:33:21Z

 // Write calls the underlying function with the supplied message
 func (o OutputFunc) Write(m *Message) error { return o(m) }

+// Close satisfies the interface with a no-op


This comment should specify "satisfies the Output interface with a no-op"

cmikk · 2026-07-14T22:40:02Z

 	// Read returns a Message or nil, and an error if any.
 	Read() (*Message, error)

+	// Closes and releases the input's resources.


The additions of the Close() method to the Input and Output interfaces and their implementations are a logically separate change and should be in its own commit, possibly along with the explicit calls to these new Close()s in the go-nmsg tests.

(The current commit description of "cgo-nmsg sometimes segfaults..." also does not match the changes.)

cmikk · 2026-07-14T23:00:50Z

 	}
 	return &nmsgOutput{output: outp}, nil
 }
+


This change to the cgo-nmsg zmq API should be handled as a separate commit, if not as a separate issue with possibly-breaking[1] API changes on the table.

The original code (mine) is kind of odd in this respect; I most likely chose this "global singleton zmq_ctx" pattern to save the caller from:

separately linking against the C zmq library, and

maintaining and passing an unsafe.Pointer (void * in the C zmq API) value

A different approach would be to expose / re-expose a zmq context API from cgo-nmsg, wrapping the zmq_ctx in a proper type, and requiring that argument to NewZMQOutput / NewZMQInput, which would be more in line with C nmsg API. Either this, the exposed unsafe.Pointer / separate linking approach would be valid alternatives to the current "let the singleton zmqContext be dropped at shutdown" approach.

FWIW, it appears that nmsgtool in the C distribution does the latter -- there are no calls to zmq_ctx_destroy or the more modern zmq_ctx_term in the nmsg distribution outside of tests.

[1] To the best of my knowledge, this cgo-nmsg code is only used within test cases in go-nmsg, as the cgo overhead is (or at least was) significant enough to prohibit most nmsg processing use cases. Thus, breaking changes are acceptable, as is living with suboptimal API features so long as they don't compromise test cases.

Would it be ok to include these two changes (zmq_ctx_new() and zmq_ctx_term()) now and introduce new APIs in a follow up?

cmikk · 2026-07-14T23:04:23Z

 	p, err := nmsg.Payload(testMessage(1000))
 	if err != nil {
-		t.Errorf(err.Error())
+		t.Error(err.Error())


These Errorf -> Error substitutions should be separately committed.

cmikk · 2026-07-14T23:07:00Z

 	{"127.0.0.1/foobar", false, 0},
 	{"127.0.0.1/390..382", false, 0},
 	{"127.0.0.1/390..foobar", false, 0},
-	{"invalid_hostname/381", false, 0},


Why was this test case removed?

The other change to this file appears purely stylistic / formatting. What was its source?

(In other projects, we're discussing standardizing on stability under the standard go fmt for simplicity.)

Reverted these

cmikk · 2026-07-14T23:20:23Z

 					return "", sockdirInvalid, SocktypeInvalid, errors.New("socket type is already set")
 				}
 				socktype = SocktypePushpull
+			default:


Note that the C library equivalent to this function (also named munge_endpoint) ignores unknown tokens. We can either leave this out for consistency or also fix the issue there (perhaps along with the lack of zmq_term in nmsgtool.)

Reverted this one too

cmikk · 2026-07-14T23:25:38Z

 			return nil, err
 		}
-		binded = true
+		bound = true


This commit has several changes to the zmq implementation mixed together. On review, I can see:

closing sockets on error

disconnecting connected sockets on close, not just bind

checking for Read() truncation, and

the editory binded -> bound

These, and any I missed, should be documented in one or more commit descriptions.

cmikk · 2026-07-14T23:34:48Z

 	if mod == nil {
 		log.Fatal("module not found")
 	}
+


As with the previous file (zmq.go), this commit mixes several changes to this file, including:

whitespace changes

Adding defer x.Close() cleanup for inputs and outputs, which could be grouped with the cgo-nmsg Close() method addition. and

adding channel synchronization to test goroutines, which is a substantive fix and should be its own commit, or at the very least documented in a commit

rafaelvanoni · 2026-07-15T01:27:36Z

@cmikk I repaved this PR with commit-separated changes, sorry for the initial mess.

This reverts commit 4a1fcb4.

rafaelvanoni requested review from cmikk and shw700 July 10, 2026 17:52

rafaelvanoni self-assigned this Jul 10, 2026

shw700 reviewed Jul 10, 2026

View reviewed changes

Comment thread go.sum Outdated

cmikk requested changes Jul 14, 2026

View reviewed changes

Rafael Vanoni added 11 commits July 14, 2026 17:21

adds Close() method to Input and Output interfaces

7499aa0

go mod updates

7099589

non-constant format string in call to (*testing.common).Errorf

6d550d2

add tags file go .gitignore

75a08f9

use new zmq_ctx_new() API; call zmq_ctx_destroy() at shutdown

2667563

s/binded/bound

bf6a14c

use Close() method for cleanup and in unit tests

9c0580d

guard against overflow in zmq_io.Read()

6007071

fail on invalid endpoint types in munge_endpoint()

4a1fcb4

use error channels and t.Fatal instead of log.Fatal in doTestMixedDo()

c204d7d

avoid footgun in TestZMQIpc

e5c07a1

rafaelvanoni force-pushed the cgo-segfault branch from 6b2dab0 to e5c07a1 Compare July 15, 2026 01:14

rafaelvanoni changed the title ~~cgo-nmsg sometimes segfaults during automated package build/testing~~ adds Close() method to Input and Output interfaces Jul 15, 2026

Rafael Vanoni added 2 commits July 14, 2026 18:39

use zmq_ctx_term() instead of zmq_ctx_destroy()

9635735

Revert "fail on invalid endpoint types in munge_endpoint()"

490008d

This reverts commit 4a1fcb4.

Uh oh!

Conversation

rafaelvanoni commented Jul 10, 2026

Uh oh!

Uh oh!

cmikk left a comment

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!

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!

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!

rafaelvanoni commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants