Issue 1153473008: A client/server helper wrapper for endpoints in Go.

Issue 1153473008: A client/server helper wrapper for endpoints in Go. (Closed)

Created:
5 years, 6 months ago by iannucci

Modified:
5 years, 6 months ago

Reviewers:
dnj, Vadim Sh.

CC:
chromium-reviews

Base URL:
https://chromium.googlesource.com/infra/infra.git@master

Target Ref:
refs/heads/master

Project:
infra

Visibility:
Public.

More Reviews

Description

A client/server helper wrapper for endpoints in Go. The epservice directory/main program generates a appengine app which links all the available endpoints implementations by finding packages which implement RegisterEndpointsService. It then runs this with dev_appserver.py. Subpackages of epservice are supposed to be one-folder-per-service. The epclient directory/main program/go-generate invocation runs the epservice server, and then runs the golang client library generator to generate go client libraries for all the services it finds. R=dnj@chromium.org, vadimsh@chromium.org BUG= Committed: https://chromium.googlesource.com/infra/infra/+/ef227d3ed8a429cce30f411570f235111633a2bd

Patch Set 1 #

Patch Set 2 : make server passed in #

Patch Set 3 : rename to better package #

Total comments: 11

Patch Set 4 : Use the real client generator! #

Total comments: 14

Patch Set 5 : refactor #

Patch Set 6 : more checks and stuff #

Total comments: 4

Patch Set 7 : channels #

Patch Set 8 : added comment #

Total comments: 3

Patch Set 9 : simplify! #

Total comments: 2

Patch Set 10 : defer stop #

Patch Set 11 : test.py stuff #

Patch Set 12 : missing upload... #

Patch Set 13 : rm newline #

Created: 5 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1596 lines, -6 lines)			Patch
M	go/Goopfile	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	go/Goopfile.lock	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epclient/api-list.json	View	1 2 3	1 chunk	+20 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epclient/dumb_counter/v1/dumb_counter-api.json	View	1 2 3	1 chunk	+218 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epclient/dumb_counter/v1/dumb_counter-gen.go	View	1 2 3	1 chunk	+386 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epclient/dumb_counter/v1/v1.infra_testing	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+4 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epclient/epclient.go	View	1 2 3 4 5	1 chunk	+113 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epclient/epclient.infra_testing	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+4 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epservice/epservice.go	View	1 2 3 4 5 6 7 8 9	1 chunk	+232 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epservice/epservice.infra_testing	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+4 lines, -0 lines	0 comments	Download
A +	go/src/infra/gae/epservice/epservice_other.go	View	1 2 3 4 5 6 7 8	1 chunk	+9 lines, -3 lines	0 comments	Download
A +	go/src/infra/gae/epservice/epservice_windows.go	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+9 lines, -3 lines	0 comments	Download
A	go/src/infra/gae/epservice/example/example.infra_testing	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+4 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epservice/example/model.go	View	1 2 3	1 chunk	+14 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epservice/example/service.go	View	1 2 3	1 chunk	+32 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epservice/example/service_add.go	View	1 2 3	1 chunk	+57 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epservice/example/service_cas.go	View	1 2 3	1 chunk	+52 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epservice/example/service_currentvalue.go	View	1 2 3	1 chunk	+43 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/epservice/example/service_list.go	View	1 2 3	1 chunk	+38 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/libs/ephelper/ephelper.go	View	1 2	1 chunk	+77 lines, -0 lines	0 comments	Download
A	go/src/infra/gae/libs/ephelper/ephelper_test.go	View	1 2	1 chunk	+138 lines, -0 lines	0 comments	Download
A	go/src/infra/libs/jsutil/jsutil.go	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+75 lines, -0 lines	0 comments	Download
A	go/src/infra/libs/jsutil/jsutil_test.go	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+63 lines, -0 lines	0 comments	Download

Messages

Total messages: 34 (10 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Vadim Sh.

http://goo.gl/2eUeWd I don't like client library that requires server side guts to work :( Honestly, ...

5 years, 6 months ago (2015-06-03 18:13:40 UTC) #2

iannucci

PostJSON only does like 30% of what this client does. The majority of the benefit ...

5 years, 6 months ago (2015-06-03 18:38:04 UTC) #3

iannucci

(Will do proper fixes when not on phone :)) And yes, it's for a standalone ...

5 years, 6 months ago (2015-06-03 18:43:03 UTC) #4

iannucci

Vadim pointed out that this is already a Solved (but Undocumented) Problem: https://github.com/google/google-api-go-client/tree/master/google-api-go-generator woohoo! I'll ...

5 years, 6 months ago (2015-06-03 19:34:39 UTC) #5

iannucci

PTAL! This uses the 'real' 'official' code generator. It has features: * epclient is an ...

5 years, 6 months ago (2015-06-07 07:32:01 UTC) #6

dnj

https://codereview.chromium.org/1153473008/diff/60001/go/src/infra/gae/epclient/dumb_counter/v1/dumb_counter-gen.go File go/src/infra/gae/epclient/dumb_counter/v1/dumb_counter-gen.go (right): https://codereview.chromium.org/1153473008/diff/60001/go/src/infra/gae/epclient/dumb_counter/v1/dumb_counter-gen.go#newcode1 go/src/infra/gae/epclient/dumb_counter/v1/dumb_counter-gen.go:1: // Package dumb_counter provides access to the . Access ...

5 years, 6 months ago (2015-06-08 16:43:18 UTC) #7

iannucci

addressed comments, ptal https://chromiumcodereview.appspot.com/1153473008/diff/60001/go/src/infra/gae/epclient/dumb_counter/v1/dumb_counter-gen.go File go/src/infra/gae/epclient/dumb_counter/v1/dumb_counter-gen.go (right): https://chromiumcodereview.appspot.com/1153473008/diff/60001/go/src/infra/gae/epclient/dumb_counter/v1/dumb_counter-gen.go#newcode1 go/src/infra/gae/epclient/dumb_counter/v1/dumb_counter-gen.go:1: // Package dumb_counter provides access to ...

5 years, 6 months ago (2015-06-09 00:01:54 UTC) #8

dnj

https://chromiumcodereview.appspot.com/1153473008/diff/100001/go/src/infra/gae/epservice/epservice.go File go/src/infra/gae/epservice/epservice.go (right): https://chromiumcodereview.appspot.com/1153473008/diff/100001/go/src/infra/gae/epservice/epservice.go#newcode113 go/src/infra/gae/epservice/epservice.go:113: `\nfunc RegisterEndpointsService\(\w* \*\w*\.Server\) error {\n`) This seems like a ...

5 years, 6 months ago (2015-06-09 01:48:14 UTC) #9

https://chromiumcodereview.appspot.com/1153473008/diff/100001/go/src/infra/ga...
File go/src/infra/gae/epservice/epservice.go (right):

https://chromiumcodereview.appspot.com/1153473008/diff/100001/go/src/infra/ga...
go/src/infra/gae/epservice/epservice.go:113: `\nfunc
RegisterEndpointsService\(\w* \*\w*\.Server\) error {\n`)
This seems like a dangerous approach. There are many possible ways to construct
this method that don't match the RE.

func RegisterEndpointsService(
  s http.Server) (
  error) { asdfasdfasdf }

Let Go do the parsing for you:
import (
        "go/ast"
        "go/parser"
        "go/token"
)

func declIsRegisterDecl(d ast.Decl) (ret bool) {
        dt, ok := d.(*ast.FuncDecl)
        if !ok {
                return
        }

        if dt.Name.String() != "RegisterEndpointsService" {
                return
        }
        // Exactly one return value: error
        if len(dt.Type.Results.List) != 1 {
                return
        }
        r0, ok := dt.Type.Results.List[0].Type.(*ast.Ident)
        if !ok {
                return
        }
        if r0.Name != "error" {
                return
        }

        // Exactly one argument: a pointer to a Server object.
        if len(dt.Type.Params.List) != 1 {
                return
        }
        p0, ok := dt.Type.Params.List[0].Type.(*ast.StarExpr)
        if !ok {
                return
        }
        p0X, ok := p0.X.(*ast.SelectorExpr)
        if !ok {
                return
        }
        if p0X.Sel.Name != "Server" {
                return
        }

        ret = true
        return
}

func hasRegisterDecl(path string) bool {
        fset := token.NewFileSet() // positions are relative to fset
        f, err := parser.ParseFile(fset, path, nil, 0)
        if err != nil {
                panic(err)
        }
        ast.Print(fset, f)

        for _, d := range f.Decls {
                if declIsRegisterDecl(d) {
                        return true
                }
        }

        return false
}

https://chromiumcodereview.appspot.com/1153473008/diff/100001/go/src/infra/ga...
go/src/infra/gae/epservice/epservice.go:230: stop()
Since you are not exiting on signal, "stop()" can get called twice, once here
and once in your main() deferred when wait() finishes, sending "os.Interrupt"
twice.

Consider:

defer stop()

signalC := make(chan struct{})
go func() {
  defer close(signalC)
  <-intChan
}

waitC := make(chan struct{})
go func() {
  defer close(waitC)
  wait()
}()

select {
case <-signalC:
  break
case <-waitC:
  break
}

iannucci

https://codereview.chromium.org/1153473008/diff/100001/go/src/infra/gae/epservice/epservice.go File go/src/infra/gae/epservice/epservice.go (right): https://codereview.chromium.org/1153473008/diff/100001/go/src/infra/gae/epservice/epservice.go#newcode113 go/src/infra/gae/epservice/epservice.go:113: `\nfunc RegisterEndpointsService\(\w* \*\w*\.Server\) error {\n`) On 2015/06/09 01:48:14, dnj ...

5 years, 6 months ago (2015-06-09 02:22:07 UTC) #10

https://codereview.chromium.org/1153473008/diff/100001/go/src/infra/gae/epser...
File go/src/infra/gae/epservice/epservice.go (right):

https://codereview.chromium.org/1153473008/diff/100001/go/src/infra/gae/epser...
go/src/infra/gae/epservice/epservice.go:113: `\nfunc
RegisterEndpointsService\(\w* \*\w*\.Server\) error {\n`)
On 2015/06/09 01:48:14, dnj wrote:
> This seems like a dangerous approach. There are many possible ways to
construct
> this method that don't match the RE.
> 
> func RegisterEndpointsService(
>   s http.Server) (
>   error) { asdfasdfasdf }
> 
> Let Go do the parsing for you:
> import (
>         "go/ast"
>         "go/parser"
>         "go/token"
> )
> 
> func declIsRegisterDecl(d ast.Decl) (ret bool) {
>         dt, ok := d.(*ast.FuncDecl)
>         if !ok {
>                 return
>         }
> 
>         if dt.Name.String() != "RegisterEndpointsService" {
>                 return
>         }
>         // Exactly one return value: error
>         if len(dt.Type.Results.List) != 1 {
>                 return
>         }
>         r0, ok := dt.Type.Results.List[0].Type.(*ast.Ident)
>         if !ok {
>                 return
>         }
>         if r0.Name != "error" {
>                 return
>         }
> 
>         // Exactly one argument: a pointer to a Server object.
>         if len(dt.Type.Params.List) != 1 {
>                 return
>         }
>         p0, ok := dt.Type.Params.List[0].Type.(*ast.StarExpr)
>         if !ok {
>                 return
>         }
>         p0X, ok := p0.X.(*ast.SelectorExpr)
>         if !ok {
>                 return
>         }
>         if p0X.Sel.Name != "Server" {
>                 return
>         }
> 
>         ret = true
>         return
> }
> 
> func hasRegisterDecl(path string) bool {
>         fset := token.NewFileSet() // positions are relative to fset
>         f, err := parser.ParseFile(fset, path, nil, 0)
>         if err != nil {
>                 panic(err)
>         }
>         ast.Print(fset, f)
> 
>         for _, d := range f.Decls {
>                 if declIsRegisterDecl(d) {
>                         return true
>                 }
>         }
> 
>         return false
> }

That would be true if we didn't enforce gofmt, but we do, so I think the regex
is a lot simpler, faster and clearer.

https://codereview.chromium.org/1153473008/diff/100001/go/src/infra/gae/epser...
go/src/infra/gae/epservice/epservice.go:230: stop()
On 2015/06/09 01:48:13, dnj wrote:
> Since you are not exiting on signal, "stop()" can get called twice, once here
> and once in your main() deferred when wait() finishes, sending "os.Interrupt"
> twice.
> 
> Consider:
> 
> defer stop()
> 
> signalC := make(chan struct{})
> go func() {
>   defer close(signalC)
>   <-intChan
> }
> 
> waitC := make(chan struct{})
> go func() {
>   defer close(waitC)
>   wait()
> }()
> 
> select {
> case <-signalC:
>   break
> case <-waitC:
>   break
> }

Done.

iannucci

(I'd also point out that dev_appserver can still get signal'd twice if you ctrl-C; once ...

5 years, 6 months ago (2015-06-09 02:25:58 UTC) #11

iannucci

On 2015/06/09 02:22:07, iannucci wrote: > https://codereview.chromium.org/1153473008/diff/100001/go/src/infra/gae/epservice/epservice.go > File go/src/infra/gae/epservice/epservice.go (right): > > https://codereview.chromium.org/1153473008/diff/100001/go/src/infra/gae/epservice/epservice.go#newcode113 > ...

5 years, 6 months ago (2015-06-09 02:27:51 UTC) #12

On 2015/06/09 02:22:07, iannucci wrote:
>
https://codereview.chromium.org/1153473008/diff/100001/go/src/infra/gae/epser...
> File go/src/infra/gae/epservice/epservice.go (right):
> 
>
https://codereview.chromium.org/1153473008/diff/100001/go/src/infra/gae/epser...
> go/src/infra/gae/epservice/epservice.go:113: `\nfunc
> RegisterEndpointsService\(\w* \*\w*\.Server\) error {\n`)
> On 2015/06/09 01:48:14, dnj wrote:
> > This seems like a dangerous approach. There are many possible ways to
> construct
> > this method that don't match the RE.
> > 
> > func RegisterEndpointsService(
> >   s http.Server) (
> >   error) { asdfasdfasdf }
> > 
> > Let Go do the parsing for you:
> > import (
> >         "go/ast"
> >         "go/parser"
> >         "go/token"
> > )
> > 
> > func declIsRegisterDecl(d ast.Decl) (ret bool) {
> >         dt, ok := d.(*ast.FuncDecl)
> >         if !ok {
> >                 return
> >         }
> > 
> >         if dt.Name.String() != "RegisterEndpointsService" {
> >                 return
> >         }
> >         // Exactly one return value: error
> >         if len(dt.Type.Results.List) != 1 {
> >                 return
> >         }
> >         r0, ok := dt.Type.Results.List[0].Type.(*ast.Ident)
> >         if !ok {
> >                 return
> >         }
> >         if r0.Name != "error" {
> >                 return
> >         }
> > 
> >         // Exactly one argument: a pointer to a Server object.
> >         if len(dt.Type.Params.List) != 1 {
> >                 return
> >         }
> >         p0, ok := dt.Type.Params.List[0].Type.(*ast.StarExpr)
> >         if !ok {
> >                 return
> >         }
> >         p0X, ok := p0.X.(*ast.SelectorExpr)
> >         if !ok {
> >                 return
> >         }
> >         if p0X.Sel.Name != "Server" {
> >                 return
> >         }
> > 
> >         ret = true
> >         return
> > }
> > 
> > func hasRegisterDecl(path string) bool {
> >         fset := token.NewFileSet() // positions are relative to fset
> >         f, err := parser.ParseFile(fset, path, nil, 0)
> >         if err != nil {
> >                 panic(err)
> >         }
> >         ast.Print(fset, f)
> > 
> >         for _, d := range f.Decls {
> >                 if declIsRegisterDecl(d) {
> >                         return true
> >                 }
> >         }
> > 
> >         return false
> > }
> 
> That would be true if we didn't enforce gofmt, but we do, so I think the regex
> is a lot simpler, faster and clearer.
> 

ugh, yes, gofmt wouldn't correct this. I still don't see the value in adding the
extra multi-dozen lines of code. I'll add a comment.

>
https://codereview.chromium.org/1153473008/diff/100001/go/src/infra/gae/epser...
> go/src/infra/gae/epservice/epservice.go:230: stop()
> On 2015/06/09 01:48:13, dnj wrote:
> > Since you are not exiting on signal, "stop()" can get called twice, once
here
> > and once in your main() deferred when wait() finishes, sending
"os.Interrupt"
> > twice.
> > 
> > Consider:
> > 
> > defer stop()
> > 
> > signalC := make(chan struct{})
> > go func() {
> >   defer close(signalC)
> >   <-intChan
> > }
> > 
> > waitC := make(chan struct{})
> > go func() {
> >   defer close(waitC)
> >   wait()
> > }()
> > 
> > select {
> > case <-signalC:
> >   break
> > case <-waitC:
> >   break
> > }
> 
> Done.

iannucci

Yes it does. I see negative value in supporting that though. On Mon, Jun 8, ...

5 years, 6 months ago (2015-06-09 02:52:46 UTC) #14

dnj

Yeah I hear 'ya. I barely care, so I'm fine with the simpler RE :P ...

5 years, 6 months ago (2015-06-09 02:56:58 UTC) #15

iannucci

much better implementation https://chromiumcodereview.appspot.com/1153473008/diff/80002/go/src/infra/gae/epservice/epservice.go File go/src/infra/gae/epservice/epservice.go (right): https://chromiumcodereview.appspot.com/1153473008/diff/80002/go/src/infra/gae/epservice/epservice.go#newcode249 go/src/infra/gae/epservice/epservice.go:249: time.Sleep(time.Second) On 2015/06/09 02:56:58, dnj wrote: ...

5 years, 6 months ago (2015-06-09 07:41:11 UTC) #16

dnj

lgtm w/ nit https://chromiumcodereview.appspot.com/1153473008/diff/150001/go/src/infra/gae/epservice/epservice.go File go/src/infra/gae/epservice/epservice.go (right): https://chromiumcodereview.appspot.com/1153473008/diff/150001/go/src/infra/gae/epservice/epservice.go#newcode229 go/src/infra/gae/epservice/epservice.go:229: stop := startServer(dir) Use "defer stop()".

5 years, 6 months ago (2015-06-09 17:17:35 UTC) #17

iannucci

https://chromiumcodereview.appspot.com/1153473008/diff/150001/go/src/infra/gae/epservice/epservice.go File go/src/infra/gae/epservice/epservice.go (right): https://chromiumcodereview.appspot.com/1153473008/diff/150001/go/src/infra/gae/epservice/epservice.go#newcode229 go/src/infra/gae/epservice/epservice.go:229: stop := startServer(dir) On 2015/06/09 17:17:35, dnj wrote: > ...

5 years, 6 months ago (2015-06-09 20:51:07 UTC) #18