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 listener problems #540

Merged
merged 6 commits into from
Jan 11, 2015
Merged

fix listener problems #540

merged 6 commits into from
Jan 11, 2015

Conversation

jbenet
Copy link
Member

@jbenet jbenet commented Jan 11, 2015

Our listeners used to hang on accept errors, becuase:

  • the listeners were bailing on secure conn fails
  • peerstream would hang after 10 temp errors

This PR fixes all that and adds some tests.

thanks so much for finding these @whyrusleeping

This should handle early breakages, where a failing connection
would take out the listener entirely.

There are probably other errors we should be handling here,
like secure connection failures.
Instead of erroring out, which would break the listener,
we instead log a message and continue. This is not an error,
the internet is a place with lots of probing + connection
failures.
peerstream would hang when it got many temporary errors.
temp errors should not count as an error. Now, it will
only exit when an error is not temporary.

I kept the acceptErr channel because it will no longer
cause a bad hang. The goroutine is exiting, so if it
blocks until acceptErr is read, it's fine. If users
launch tons of listers and see goroutines building up,
they know they should be reading + logging those.
@jbenet jbenet added the status/in-progress In progress label Jan 11, 2015

for {
conn, err := listener.Accept()
if err != nil && c.IsTemporary(c) {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like were throwing errors away here that we may want to at least note somehow... maybe it should also take a func(err error) so we can log those errors if we want?

@btc
Copy link
Contributor

btc commented Jan 11, 2015

It's a bit inconvenient to do meaningful CR on a PR when most of the new code exists in other repos.

(It's less of an issue when the outside libraries are mature. However, when vendored code is new, we should expect one or two bugs to exist.)

(Would be less of a concern if outside libraries are on a GitHub org with the same ACLs as this repo and we were to get into the practice of doing code reviews on the libraries on an individual basis.)

@jbenet
Copy link
Member Author

jbenet commented Jan 11, 2015

It's a bit inconvenient to do meaningful CR on a PR when most of the new code exists in other repos.

Would it help if i linked to the relevant commit diffs?

(Would be less of a concern if outside libraries are on a GitHub org with the same ACLs as this repo)

this is feasible for some things, like peerstream. this is not feasible for everything, i will make tons of small, modular packages that transcend belonging to an org.

I think we see things very differently on when to modularize. to help align our perspectives, here's some stuff that helps express how i approach this:

my thoughts are not the exact same substack's, but we align very well.

We should identify which steps of modularizing things can be automated and which costs can be decreased. I actually timed myself modularizing go-temp-err-catcher:

27min - write it inside go-peerstream/catcher
5min - pull into own repo (readme) (using `hub`)
5min - cleaned up interface to be more user friendly + docs
10min - good tests - i would have not written these. 
            i actually found an error and added a feature
3min - setup travis (`cp ../other/.travis.yml .travis.yml`) 
         - enable it on the browser (may be able to curl this)
1min - forgot license (`cp ../other/LICENSE LICENSE`)
3min - adding godep + make vendor
6min - pulling in new changes
---------
27min - writing it
33min - modularizing

The total cost of modularizing it was a bit more than writing it in the first place, but i found an error while writing tests that probably saved us many hours down the road.

The major costs for me were not actually vendoring, as i expected, but the rote work of godoc + package setup + tests. there may be a script that could automate this dramatically.

usage: go-modularize <path>

extracts package at <path> into its own module:
- runs `{go fmt, golint, go vet}` ensuring you write good code
- creates a git repository at github.com/<username>/go-<packagename>
- adds a README.md file with godoc + travis badge
- adds a LICENSE file (use --license to override default ~/.go-modularize/LICENSE)
- adds a .travis.yml file (skipped with --skip-travis and --skip-tests)
- ensures there are tests. will error + yell at you if there are none (skipped with --skip-tests)
- ships the module, if everything succeeded, uses `hub create` and `git push`

other things tis could do:
- godep vendors modules
- coveralls

On the CR costs, perhaps what can help is:

  • include links to relevant commits (the diff is included here, so this is actually no different than doing it locally.
  • with godeps, we can do comments in here to other packages, like @whyrusleeping did in my ds-query diff

As mentioned above, a github org will not work for everything and it will not work with other contributors, so we should not rely on it as a pre-requisite for modularity.

jbenet added a commit that referenced this pull request Jan 11, 2015
@jbenet jbenet merged commit dac2e44 into master Jan 11, 2015
@jbenet jbenet deleted the listener-fixes branch January 11, 2015 21:43
@jbenet jbenet removed the status/in-progress In progress label Jan 11, 2015
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
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