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

Move some handle request functions up a level #5889

Merged
merged 2 commits into from Dec 3, 2018
Merged

Move some handle request functions up a level #5889

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04cd15c
Move some handle request functions up a level
jefferai Dec 3, 2018
3aad6e7
Update request_handling.go
jefferai Dec 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 22 additions & 9 deletions vault/request_handling.go
View file
Open in desktop
Expand Up @@ -345,17 +345,16 @@ func (c *Core) checkToken(ctx context.Context, req *logical.Request, unauth bool
// HandleRequest is used to handle a new incoming request
func (c *Core) HandleRequest(httpCtx context.Context, req *logical.Request) (resp *logical.Response, err error) {
c.stateLock.RLock()
defer c.stateLock.RUnlock()
if c.Sealed() {
c.stateLock.RUnlock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not a defer unlock? Performance?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah defers are really slow. At some point I went through and got rid of a lot of defers in the request path and shaved off tens of microseconds, which was a pretty significant percentage of total request time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically if we can easily avoid defers in the fiery hot path it's absolutely worth it to do so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's fair. I waffle back and forth between worrying about the performance hit vs worrying about missing a code path.

return nil, consts.ErrSealed
}
if c.standby && !c.perfStandby {
c.stateLock.RUnlock()
return nil, consts.ErrStandby
}

ctx, cancel := context.WithCancel(c.activeContext)
defer cancel()

go func(ctx context.Context, httpCtx context.Context) {
select {
case <-ctx.Done():
Expand All @@ -364,6 +363,26 @@ func (c *Core) HandleRequest(httpCtx context.Context, req *logical.Request) (res
}
}(ctx, httpCtx)

ns, err := namespace.FromContext(httpCtx)
if err != nil {
cancel()
c.stateLock.RUnlock()
return nil, errwrap.Wrapf("could not parse namespace from http context: {{err}}", err)
}
ctx = namespace.ContextWithNamespace(ctx, ns)

resp, err = c.handleCancelableRequest(ctx, ns, req)

if req != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If req can be nil, we probably shouldn't be invoking c.handleCancelableRequest with it. Will panic when trying to access req.Path.

Copy link
Member Author

Choose a reason for hiding this comment

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

It can't be. In theory. :-) I can remove that, sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool

req.SetTokenEntry(nil)
}

cancel()
c.stateLock.RUnlock()
return resp, err
}

func (c *Core) handleCancelableRequest(ctx context.Context, ns *namespace.Namespace, req *logical.Request) (resp *logical.Response, err error) {
// Allowing writing to a path ending in / makes it extremely difficult to
// understand user intent for the filesystem-like backends (kv,
// cubbyhole) -- did they want a key named foo/ or did they want to write
Expand All @@ -381,12 +400,6 @@ func (c *Core) HandleRequest(httpCtx context.Context, req *logical.Request) (res
return nil, err
}

ns, err := namespace.FromContext(httpCtx)
if err != nil {
return nil, errwrap.Wrapf("could not parse namespace from http context: {{err}}", err)
}
ctx = namespace.ContextWithNamespace(ctx, ns)

if !hasNamespaces(c) && ns.Path != "" {
return nil, logical.CodedError(403, "namespaces feature not enabled")
}
Expand Down