Skip to content
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.

Sign up for GitHub

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

Jump to bottom

zmq4: add option for automatic reconnect #127

Merged
merged 1 commit into from Jun 21, 2022
Merged

zmq4: add option for automatic reconnect #127

merged 1 commit into from Jun 21, 2022

Conversation

thielepaul
Copy link
Contributor

@thielepaul thielepaul commented Jun 9, 2022 •
edited by sbinet

I have not tested this yet, it's meant as discussion base for #124

Fixes #124

@thielepaul thielepaul mentioned this pull request Jun 9, 2022
Closed
@thielepaul
Copy link
Contributor Author

I tested it for pub and sub sockets: for pub sockets it works fine, for sub socket I have the problem described in #116

@codecov
Copy link

codecov bot commented Jun 13, 2022 •
edited

Codecov Report

Merging #127 (a2090eb) into main (ae18bc0) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head a2090eb differs from pull request most recent head 978883a. Consider uploading reports for the commit 978883a to get more accurate results

@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
- Coverage   67.58%   67.58%   -0.01%     
==========================================
  Files          29       29              
  Lines        1820     1823       +3     
==========================================
+ Hits         1230     1232       +2     
- Misses        490      491       +1     
  Partials      100      100
Impacted Files Coverage Δ
options.go 66.66% <100.00%> (-16.67%) ⬇️
socket.go 84.18% <100.00%> (+0.27%) ⬆️
internal/inproc/inproc.go 72.15% <0.00%> (+1.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae18bc0...978883a. Read the comment docs.

sbinet
sbinet reviewed Jun 13, 2022
View reviewed changes
Copy link
Contributor

@sbinet sbinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good.

thanks for the PR.

socket.go Outdated Show resolved Hide resolved
socket.go Outdated
onCloseCb = func(c *Conn) {
sck.scheduleRmConn(c)
time.Sleep(sck.retry)
sck.Dial(endpoint)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calling back into socket.Dial will start another connReaper goroutine.
are we sure this is ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, this is a problem. I just added a reaperStartedbool flag. Do you think this is an ok approach to fix this?

options.go Show resolved Hide resolved
@thielepaul thielepaul force-pushed the automatic-reconnect branch 3 times, most recently from e308511 to 6d219d4 Compare June 15, 2022 20:43
@sbinet
Copy link
Contributor

sbinet commented Jun 17, 2022

please rebase off the latest main, thanks.

@thielepaul thielepaul force-pushed the automatic-reconnect branch 3 times, most recently from d0375cc to 8903a75 Compare June 17, 2022 11:37
@sbinet
Copy link
Contributor

sbinet commented Jun 20, 2022

please rebase off the latest main, thanks.
(yeah, stacked PRs are a bit of a pain on github.)

@thielepaul thielepaul marked this pull request as ready for review June 20, 2022 09:28
@thielepaul
Copy link
Contributor Author

thielepaul commented Jun 20, 2022 •
edited

please rebase off the latest main, thanks. (yeah, stacked PRs are a bit of a pain on github.)

No problem, thank you for taking the time to review my PRs!

sbinet
sbinet reviewed Jun 20, 2022
View reviewed changes
Copy link
Contributor

@sbinet sbinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last remaining nit-picks.
looking good :)

socket_test.go Outdated Show resolved Hide resolved
socket_test.go Show resolved Hide resolved
socket_test.go Outdated Show resolved Hide resolved
@thielepaul
Copy link
Contributor Author

last remaining nit-picks. looking good :)

thx, I applied them

@sbinet sbinet merged commit 2b7cf28 into go-zeromq:main Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbinet sbinet sbinet left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

no reconnect possible when using zmq4.NewPub with socket.Dial
2 participants
@thielepaul @sbinet