Issue #108: moved subscribe logic from pub to sub and xsub sockets. #112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sbinet
reviewed
Sep 16, 2021
sbinet
reviewed
Sep 27, 2021
There was a problem hiding this comment.
one last wrinkle to iron out.
could you also send another PR against go-zeromq/license adding yourself to the AUTHORS and/or CONTRIBUTORS files?
thanks again.
sbinet
reviewed
Sep 27, 2021
|
thanks for the update. we're nearly there :) |
|
Sorry, didn't notice there was another call to NewProxy(). Let's hope this
time's the charm. :-)
…On Mon, Sep 27, 2021 at 12:30 PM Sebastien Binet ***@***.***> wrote:
thanks for the update.
it seems the tests fail to compile:
# github.com/go-zeromq/zmq4_test [github.com/go-zeromq/zmq4.test]
Error: ./proxy_test.go:321:24: multiple-value zmq4.NewProxy() in single-value context
FAIL github.com/go-zeromq/zmq4 [build failed]
FAIL
Error: Process completed with exit code 2.
we're nearly there :)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC25E3HKJXI42VZ4EMPWEYDUECL2ZANCNFSM5D6JXQ4A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
two remaining issues: and the linter detected: |
|
The stuff in conn.go was me trying something - wasn't intending to commit
it. Anyway, removed it and added Topics to pub and xsub.
…On Mon, Sep 27, 2021 at 12:43 PM Sebastien Binet ***@***.***> wrote:
two remaining issues:
=== RUN TestTopics
--- FAIL: TestTopics (0.50s)
panic: interface conversion: *zmq4.pubSocket is not zmq4.Topics: missing method Topics [recovered]
panic: interface conversion: *zmq4.pubSocket is not zmq4.Topics: missing method Topics
and the linter detected:
Error: func `(*Conn).subscribed` is unused (unused)
Error: func `(*Conn).subscribe` is unused (unused)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC25E3AM46CJ5K3CYFOX76TUECNMRANCNFSM5D6JXQ4A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Sigh...copypasta gets me every time. :-/ |
|
So I removed "topics" from the Conn object because it doesn't look like it's needed -- the sub and xsub sockets have their own topics. But socket.topics() looks at that...so just trying to figure out if there really needs to be a topics field in Conn. |
|
Also, is there a way I can run this lint check? |
|
the but running originally, I don't remember off-hand whether a |
|
"go test" fails for me:
# zmq4_test [zmq4.test]
./proxy_test.go:24:47: undefined: zmq4.Devnull
./proxy_test.go:25:47: undefined: zmq4.Devnull
./proxy_test.go:26:47: undefined: zmq4.Devnull
./proxy_test.go:27:47: undefined: zmq4.Devnull
./proxy_test.go:28:47: undefined: zmq4.Devnull
./proxy_test.go:29:47: undefined: zmq4.Devnull
./proxy_test.go:291:47: undefined: zmq4.Devnull
./proxy_test.go:292:47: undefined: zmq4.Devnull
./proxy_test.go:293:47: undefined: zmq4.Devnull
./proxy_test.go:294:47: undefined: zmq4.Devnull
./proxy_test.go:294:47: too many errors
FAIL zmq4 [build failed]
FAIL
…On Tue, Sep 28, 2021 at 4:57 AM Sebastien Binet ***@***.***> wrote:
the lint stuff is a bit cumbersome to run locally.
but running go test -v ./... should catch most of what the linter
complains about.
originally, (x)pub and (x)sub would both know the topics they were
(resp.) publishing or subscribing via this socket.topics.
with the new scheme, it seems to me only (x)sub know.
so... perhaps we should just drop Conn.topics and socket.topics
altogether if not knowing/implementing Topics for (x)pub is alright.
I don't remember off-hand whether a (x)pub socket is expected to know the
topics it is publishing.
if it's alright, then we can drop that.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC25E3D5APJSBPRP2S7FEL3UEF7ORANCNFSM5D6JXQ4A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
that's quite strange: what version of Go are you using? |
|
$ go version
go version go1.17.1 darwin/amd64
…On Tue, Sep 28, 2021 at 3:45 PM Sebastien Binet ***@***.***> wrote:
that's quite strange:
$> cat zall_test.go
// Copyright 2020 The go-zeromq Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
package zmq4
import (
"io/ioutil"
"log"
)
var (
Devnull = log.New(ioutil.Discard, "zmq4: ", 0)
)
what version of Go are you using?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC25E3CIXUB3ZY5DHLWVFRDUEILPLANCNFSM5D6JXQ4A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
added 6 commits
September 28, 2021 21:56
…y don't have any subscriptions.
|
Okay, so the tests pass now. The latest changes:
- Removed topics from main socket type.
- Made pub and xpub Topics() functions return an empty list since those
socket types don't have subscriptions.
- Fixed xsub Topics() to return its topics list (it was returning the one
from the main socket).
- Removed topics tests for pub and xpub sockets in pub/sub and xpub/xsub
tests.
- Added topics test for sub socket in xpub/xsub test.
Crossing fingers that lint likes this one....
…On Tue, Sep 28, 2021 at 4:57 AM Sebastien Binet ***@***.***> wrote:
the lint stuff is a bit cumbersome to run locally.
but running go test -v ./... should catch most of what the linter
complains about.
originally, (x)pub and (x)sub would both know the topics they were
(resp.) publishing or subscribing via this socket.topics.
with the new scheme, it seems to me only (x)sub know.
so... perhaps we should just drop Conn.topics and socket.topics
altogether if not knowing/implementing Topics for (x)pub is alright.
I don't remember off-hand whether a (x)pub socket is expected to know the
topics it is publishing.
if it's alright, then we can drop that.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC25E3D5APJSBPRP2S7FEL3UEF7ORANCNFSM5D6JXQ4A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
I totally do not understand this line (which is what seems to be a problem for the build): got := sub.(zmq4.Topics).Topics() (zmq4_xpubsub_test.go:147) |
|
this does an interface-cast: it "casts" the the pure-Go you seem to have moved that code from the I guess replacing that snippet of code from got, ok := sub.(zmq4.Topics).Topics() // C++ sockets do not implement the zmq4.Topics interface.
want := []string{topics[isub]}
if ok && !reflect.DeepEqual(got, want) {
t.Fatalf("Missing or wrong topics.\ngot= %q\nwant=%q", got, want)
} |
|
Might need something like this instead:
topic, ok := sub.(zmq4.Topics) // C++ sockets do not implement the zmq4.Topics interface.
want := []string{topics[isub]}
if ok {
if got := topic.Topics(); !reflect.DeepEqual(got, want) {
t.Fatalf("Missing or wrong topics.\ngot= %q\nwant=%q", got, want)
}
}
…On Wed, Sep 29, 2021 at 11:27 AM Sebastien Binet ***@***.***> wrote:
this does an interface-cast: it "casts" the sub variable to the
zmq4.Topics interface and then call the Topics() method of that interface.
the pure-Go sub sockets do implement the Topics interface.
but the shims to the C++ ones do not.
you seem to have moved that code from the TestTopics func (that's only
run on pure-Go sockets) to the TestXPubSub one, that does exercize the
Go/Go, Go/C++, C++/Go and C++/C++ cases.
I guess replacing that snippet of code from TextXPubSub with:
got, ok := sub.(zmq4.Topics).Topics() // C++ sockets do not implement the zmq4.Topics interface.want := []string{topics[isub]}if ok && !reflect.DeepEqual(got, want) {
t.Fatalf("Missing or wrong topics.\ngot= %q\nwant=%q", got, want)
}
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC25E3ALZ3UPUITH27WB373UEMV4VANCNFSM5D6JXQ4A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Codecov Report
@@ Coverage Diff @@
## master #112 +/- ##
==========================================
- Coverage 68.08% 65.39% -2.70%
==========================================
Files 29 29
Lines 1811 1832 +21
==========================================
- Hits 1233 1198 -35
- Misses 479 534 +55
- Partials 99 100 +1
Continue to review full report at Codecov.
|
|
Ok, I have no idea why the build is still failing or what the "Codecov Report" means. |
|
It seems one of the tests deadlocks, but only with 1.17... Oh well... I'll have a look next week: I am off the grid starting now until Wednesday next week. In the meantime, could send another pull request against the 'license' repository, adding yourself to the 'AUTHORS' and/or 'CONTRIBUTORS' ? Apologies for the bumpy ride and delay(s) |
|
How likely am I to be inviting (more) spam by adding my email?
…On Wed, Sep 29, 2021 at 5:04 PM Sebastien Binet ***@***.***> wrote:
It seems one of the tests deadlocks, but only with 1.17...
Oh well...
I'll have a look next week: I am off the grid starting now until Wednesday
next week.
In the meantime, could send another pull request against the 'license'
repository, adding yourself to the 'AUTHORS' and/or 'CONTRIBUTORS' ?
When I am back I'll merge the 2 PRs (after having fixed the locked test,
unless you beat me to it).
Apologies for the bumpy ride and delay(s)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#112 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC25E3CQ3B2QLTPTT3XOQX3UEN5PBANCNFSM5D6JXQ4A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
(I haven't forgotten about this PR. but |
|
ok, so...
well, I haven't seen any uptick of spam in my inbox although I have my address in a few if you prefer, you could just put your name + GitHub handle. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also added a subscribe all in proxy.go, because otherwise it won't work with pub/sub sockets. This has no effect for other socket types.