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

context.Context as the first parameter of all RPC calls #1741

Merged
merged 18 commits into from
Apr 19, 2016
Merged

context.Context as the first parameter of all RPC calls #1741

merged 18 commits into from
Apr 19, 2016

Conversation

riking
Copy link
Contributor

@riking riking commented Apr 15, 2016

see also #1292

@riking riking added the r? label Apr 18, 2016
jsha
jsha reviewed Apr 18, 2016
View reviewed changes
ca/certificate-authority_test.go
m.certificate.DER = der
return "", nil
}

var caKey crypto.Signer
var caCert *x509.Certificate
var contxt = context.Background()
Copy link
Contributor

Choose a reason for hiding this comment

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

In the blog post at https://blog.golang.org/context, they just inline multiple calls to context.Background(). From reading the documentation it sounds like either is fine, but picking an unusual misspelling of context to work around scoping is a net loss. I think we should just inline context.Background() here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternately: It looks like you're avoiding using the name ctx, possibly because it's used as an arg to AddCertificate. But I think it would be fine to use ctx. You can also use _ for AddCertificate's param.

jsha
jsha reviewed Apr 18, 2016
View reviewed changes
cmd/ocsp-updater/main.go
@@ -501,7 +503,8 @@ type looper struct {

func (l *looper) tick() {
tickStart := l.clk.Now()
err := l.tickFunc(l.batchSize)
ctx := context.TODO()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why TODO rather than Background?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be useful to have a maximum time for a tick, so I wasn't quite sure that Background was appropriate.

jsha
jsha reviewed Apr 18, 2016
View reviewed changes
policy/policy-authority.go
@@ -271,7 +272,7 @@ func (pa *AuthorityImpl) checkHostLists(domain string) error {
// acceptable for the given identifier.
//
// Note: Current implementation is static, but future versions may not be.
func (pa *AuthorityImpl) ChallengesFor(identifier core.AcmeIdentifier, accountKey *jose.JsonWebKey) ([]core.Challenge, [][]int) {
func (pa *AuthorityImpl) ChallengesFor(ctx context.Context, identifier core.AcmeIdentifier, accountKey *jose.JsonWebKey) ([]core.Challenge, [][]int) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function doesn't need a context since it doesn't call any RPCs or do anything that needs timing out.

rolandshoemaker
rolandshoemaker reviewed Apr 18, 2016
View reviewed changes
cmd/cert-checker/main.go
@@ -141,9 +143,9 @@ func (c *certChecker) getCerts(unexpiredOnly bool) error {
return nil
}

func (c *certChecker) processCerts(wg *sync.WaitGroup, badResultsOnly bool) {
func (c *certChecker) processCerts(ctx context.Context, wg *sync.WaitGroup, badResultsOnly bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think Contexts are needed here, we don't cross any RPC boundaries and they aren't used anywhere.

@rolandshoemaker
Copy link
Contributor

Other than my two comments I think this looks good, it's very big though.

@riking riking merged commit b7cf618 into master Apr 19, 2016
@riking riking deleted the add-ctx branch April 19, 2016 18:34
jsha added a commit that referenced this pull request Apr 25, 2016
@jsha
Merge master into staging (#1765)
c9bfbd4 
* Switch to new vendor style (#1747)

* Switch to new vendor style.

* Fix metrics generate command.

* Fix miekg/dns types_generate.

* Use generated copies of files.

* Update miekg to latest.

Fixes a problem with `go generate`.

* Set GO15VENDOREXPERIMENT.

* Build in letsencrypt/boulder.

* fix travis more.

* Exclude vendor instead of godeps.

* Replace some ...

* Fix unformatted cmd

* Fix errcheck for vendorexp

* Add GO15VENDOREXPERIMENT to Makefile.

* Temp disable errcheck.

* Restore master fetch.

* Restore errcheck.

* Build with 1.6 also.

* Match statsd.*"

* Skip errcheck unles Go1.6.

* Add other ignorepkg.

* Fix errcheck.

* move errcheck

* Remove go1.6 requirement.

* Put godep-restore with errcheck.

* Remove go1.6 dep.

* Revert master fetch revert.

* Remove -r flag from godep save.

* Set GO15VENDOREXPERIMENT in Dockerfile and remove _worskpace.

* Fix Godep version.

* Use tools/venv.sh from client repo in integration tests (#1752)

* Fix total DNS latency stat (#1751)

exchangeOne used a deferd method which contained a expression as a argument. Because of how defer works the arguments where evaluated immediately (unlike the method) causing the total latency to always be the same.

* Rework how KeyAuthorization works (#1688)

* Enhance error message in NewKeyAuthFromString

* va: generate expected response and string compare

* NewKeyAuth can return error, handle that...

* wip commit

* convert ch.KeyAuthorization to string and rename

* more wip commit

* wip 3

* wip 4

* delete NewKeyAuthorizationFromString

* change to ServerInternalProblem

* fix compile error

* semantic merge conflicts are the worst

actually compiled this time.

* Replace new error with statsd increment

* Parallelise email DNS lookup for new-reg (#1731)

* Parallelise email DNS lookup for new-reg

* Remove the 1-case switch

* context.Context as the first parameter of all RPC calls (#1741)

Change core/interfaces to put context.Context as the first parameter of all RPC calls in preparation for gRPC.

* Update grpc dep and regenerate caa-checker. (#1761)

* Update grpc dep and regenerate caa-checker.

The latest version generates a different format. This is a precursor to running
go generate in Travis.

* Run go generate in Travis (#1762)

* Fix go generate command in metrics.

The previous command only worked on OS X. This one works on Linux but not
OS X.

Also add generate phase of test.sh.

* Add mockgen to test setup.

* Fix github-pr-status output.

* Fix envvar style.

* Set xtrace.

* Fix test.sh

* Fix test.sh some more.

* Fix mockgen command.

* Add dependencies for running `go generate`.

* Add protoc-gen-go.

* Fix go get command.

* Fix generate.

* Wait for all.

* Fix generate.

* Update generated pb.

* Fix generate commands for vendored world.

* Update documentation for new vendor style.

* Update grpc package to latest.

* Update caaChecker proto with latest.

* Run go generate only over TESTPATHS

* See if Travis passes under 1.6

* Switch back to 1.5.

* Trim run command.

* Run stringer from correct directory.

* Move generate command.

* Restore and generate

* Fix path.

* list contents of GOPATH.

* Fix stringer by prebuilding.

* Try another import path.

* regenerate bcode_string.

* remove excess package

* pull jsha fork of protoc-gen-go that echoes

* Echo protoc version.

* install from source

* CD back.

* Go back to normal protoc-gen-go

* Fix path

* Move protobuf install into test/setup.sh

* Move before_install to install.

* Set PATH.

* Follow 301 with curl.

* Shuffle test order.

* Swap back test order.

* Restore all tests.

* Restore 1.5.3 to Travis.

* Remove unnecessary wait-or-exit

* Generate metrics mock with latest mockgen.

* Wrap TESTPATHS in curlies

* Remove spurious bracket
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
r=jsha r=rolandshoemaker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@riking @rolandshoemaker @jsha