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
Add a Middleware stack to zanzibar runtime [1/3] #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change the middleware interface to be more friendly for performance and debugging concerns.
Encouraging middleware authors to use closures is going to hurt us in the long run.
@@ -28,17 +28,19 @@ imports: | |||
- name: github.com/mailru/easyjson | |||
version: 9d6630dc8c577b56cb9687a9cf9e8578aca7298a | |||
subpackages: | |||
- bootstrap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed from alphabetical to not alphabetical :(
Not sure what tool is doing this...
@@ -36,7 +36,3 @@ import: | |||
version: a689eb3bc4b53af70390acc3cf68c9f549b6b8d6 | |||
- package: github.com/buger/jsonparser | |||
version: 5b691c8ebc4af5191baa426561d62f1b5115d6e5 | |||
testImport: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does glide support test imports ? what is the correct way of doing it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated glide and it seems to be working now.
@@ -64,6 +63,10 @@ function parseFunction(folderName, functionInfo) { | |||
}; | |||
fileObj.fnCounter++; | |||
|
|||
// TODO(sindelar): Investigate | |||
if (functionInfo.Statements == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, no idea what that is.
runtime/middlewares.go
Outdated
// Handler is an interface that compatable middlewares must implement to be used | ||
// in the middleware stack. Handle should yield to the next middleware in the stack | ||
// by invoking the next HandlerFn passed in. | ||
type Handler interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This middleware interface is not very performant or debuggable.
We do not want people to author middlewares as closures. Generally writing closures instead of structs with methods is less performant and harder to debug because state is stored as closed over variables instead of fields on structs.
Having a wrapping style middleware makes reasoning about the performance profile and flamegraphs difficult.
Currently our flamegraphs look something like this :
If we wrap each middleware as a closure like this each middleware will add at least one new stack frame, eventually it will look something like this :
We want to avoid all of this noise in the stack frames so it's best if middlewares were a struct with a HandleRequest() / HandleResponse() interface. We then need a middleware runner that loops over the middlewares and calls them one by one and then finally calls the endpoint once all the middlewares are done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I am going to setup a version that iteratively calls interfaces instead of passing the functions through.
This approach will break defers and context scoping though.
For instance, a middleware that performs an action with a database connection or file handle and closes it on defer will never be called if a lower level middleware panics or times out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can still work, just not using defer
itself.
If a middleware's HandleRequest()
is called then the middlewares HandleResponse()
will be called. You can store some state on the middleware handle instance in HandleRequest()
and explicitly clean it up in HandleResponse()
As for panic safety, if something panics then the process is in an undefined state and we have to restart it, I don't know if we can have panic safety for all code in the codebase.
I'm not familiar enough to talk about context scoping but it sounds like the middlewares and endpoints will all have the same context managed by the router/server itself.
runtime/middlewares.go
Outdated
|
||
// DefaultStack returns a new middleware stack instance with the default middlewares. | ||
// | ||
// Logger - Request/Response Logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although logger and tracer are useful examples of middlewares.
we might be better off for performance/debugging reasons to implement them as methods on the ServerReq / ServerRes class.
runtime/middlewares.go
Outdated
// MiddlewareHandle used to define middleware | ||
type MiddlewareHandle interface { | ||
// implement HandleRequest for your middleware. | ||
HandleRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should pass ctx into the HandleRequest
and HandleResponse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
req *zanzibar.ServerHTTPRequest, | ||
res *zanzibar.ServerHTTPResponse, | ||
shared zanzibar.SharedState) error { | ||
m.middlewareState = MiddlewareState{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This middlewareState
is re-used for all requests to one endpoint.
We need to find some way to store "per request" state.
runtime/middlewares.go
Outdated
sharedState.middlewareDict = make(map[string]interface{}) | ||
|
||
for i := 0; i < len(middlewares); i++ { | ||
sharedState.middlewareDict[middlewares[i].Name()] = middlewares[i] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we initialize this to nil
?
runtime/middlewares.go
Outdated
shared := newSharedState(m.middlewares) | ||
|
||
for i := 0; i < len(m.middlewares); i++ { | ||
err := m.middlewares[i].HandleRequest(ctx, req, res, shared) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add TODO: "A middleware needs to be able to return a bool to say that it has written to the response"
Consider not allowing middlewares to return error
. When an error
is returned its ambiguous about who should write to the response.
runtime/middlewares.go
Outdated
m.handle(ctx, req, res) | ||
|
||
for i := len(m.middlewares) - 1; i >= 0; i-- { | ||
err := m.middlewares[i].HandleResponse(ctx, res, shared) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add TODO: "A middleware needs to be able to return a bool to say that it has written to the response"
@@ -0,0 +1,67 @@ | |||
// Copyright (c) 2017 Uber Technologies, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file does not implement new middleware interface, either refactor or delete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} | ||
|
||
// SharedState used to access other middlewares in the chain. | ||
type SharedState struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add more fields in SharedState
? why not type SharedState map[string]interface{}
?
} | ||
|
||
// HandleRequest handles the requests before calling lower level middlewares. | ||
func (m exampleMiddleware) HandleRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be m *exampleMiddleware
. *exampleMiddleware
doesn't implement the middleware interface if you define the method not on pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
return nil | ||
} | ||
|
||
func (m exampleMiddleware) HandleResponse( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
schema := (&exampleMiddleware{}).JSONSchema() | ||
|
||
// TODO(sindelar): Read expected from test file | ||
expected := "{\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use ``
as the outmost quotes for raw string to avoid type so many escapes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip. I'm going to change this to read from the golden files in a follow up, but in the future will use that approach.
This PR adds a middleware stack to the zanzibar runtime that operates on the zanzibar requests/response types. It accepts middlewares that implement the
zanzibar.HandlerFn
interface.func(ctx context.Context, req *zanzibar.ServerHTTPRequest, res *zanzibar.ServerHTTPResponse)
For middlewares that share state, the will be instantiated in the register.go file in a follow up PR. They're built using
func NewMiddleWare( gateway *zanzibar.Gateway, options Options, next zanzibar.HandlerFn) zanzibar.HandlerFn
and state can be passed through the context to the
next
middleware or handler.For middlewares that do not share state, they can be directly added to the middlestack and are executed in order. This is intended for both simple default middlewares(i.e. logging, tracing) as well as being used as helper functions for debugging and unit testing.