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

Connection Loss Improvement #1412

Closed
wants to merge 7 commits into from
Closed

Connection Loss Improvement #1412

wants to merge 7 commits into from

Conversation

janxious
Copy link
Contributor

@janxious janxious commented Jun 23, 2016

What & Why

  • Separate hello and authenticate functions
  • Force connection close at end of write cycle so we don't hold open idle connections, which has the benefit of mostly removing the chance of getting hopelessly connection lost when either end of the connection innermittently drops

Required for all PRs:

  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

@jasonroelofs pinging you since you wrote the first version

janxious added 5 commits June 23, 2016 16:46
…end of write cycle so we don't hold open idle connections, which has the benefit of mostly removing the chance of getting hopelessly connection lost
correct URL from instrumental fork to origin and put the change in the correct part of the file
@jasonroelofs
Copy link
Contributor

Looks good to me. Probably what I would have eventually done. I've tried a number of different ways to try to catch disconnects but haven't gotten anything to work reliably.

@janxious
Copy link
Contributor Author

Yeah, we tried everything we could for a couple days, but it seems like the golang primitives are not built with dealing with disconnection in mind. We have some internal work on instrumental going that will allow for ack'ing, which will simplify disconnect detection and we can circle back round to this output. Thanks for looking!

@sparrc
Copy link
Contributor

sparrc commented Jun 25, 2016

you need to run go fmt ./... on your code

@janxious
Copy link
Contributor Author

janxious commented Jul 8, 2016

@Sparcc This has been go fmt ./...'ed. Thanks for the heads up.

@sparrc
Copy link
Contributor

sparrc commented Jul 14, 2016

thanks @janxious!

@sparrc sparrc closed this in 21add2c Jul 14, 2016
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.

3 participants