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.

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

Fix agent socket cleanup #343

Merged
merged 4 commits into from
Sep 13, 2023
Merged

Fix agent socket cleanup #343

merged 4 commits into from
Sep 13, 2023

Conversation

rsafonseca
Copy link
Contributor

Fixes #289
Resolves several test flakes
Ensures Acceptors are listening before moving forward with initialization

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6172031201

  • 8 of 20 (40.0%) changed or added relevant lines in 4 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.08%) to 61.722%

Changes Missing Coverage Covered Lines Changed/Added Lines %
acceptor/tcp_acceptor.go 0 6 0.0%
acceptor/ws_acceptor.go 3 9 33.33%
Files with Coverage Reduction New Missed Lines %
modules/binary.go 2 73.91%
Totals Coverage Status
Change from base Build 6164253248: -0.08%
Covered Lines: 4797
Relevant Lines: 7772

💛 - Coveralls

@@ -160,6 +170,7 @@ func (w *WSAcceptor) serve(upgrader *websocket.Upgrader) {

// Stop stops the acceptor
func (w *WSAcceptor) Stop() {
w.running = false
Copy link
Contributor

Choose a reason for hiding this comment

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

only if the below lines didn't error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

once you issue the Stop() it should be marked as not running anyway, since there might be an error where it couldn't run close() because it was already closed or other issue, and in that situation, the acceptor would keep getting marked as running, when it really wasn't (if it can't even close itself, it should not have running=true). This matches the existing implementation on the TCPAcceptor

app.go Outdated
for a.IsRunning() == false {
logger.Log.Infof("Waiting for TCP acceptor %s to start on addr %s", reflect.TypeOf(a), a.GetConfiguredAddress())
}

logger.Log.Infof("listening with acceptor %s on addr %s", reflect.TypeOf(a), a.GetAddr())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also change this line to print configured address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure i follow, it already does

Copy link
Contributor Author

@rsafonseca rsafonseca Sep 13, 2023

Choose a reason for hiding this comment

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

If you're talking about the line that i didn't touch, it's better to keep it as it is. The configured address with print for example 0.0.0.0:0 (while the socket isn't up) and the other one will print the actual port that was used for the socket. This is useful in case for example if you don't specify a port (like on tests for example, but potentially in real life too). It will also print the IP that it was bound to on the local network stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this log line is broken though, it's not properly printing the port

@felipejfc felipejfc merged commit 1156831 into main Sep 13, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: connect not close when Handshake failed
3 participants