-
Notifications
You must be signed in to change notification settings - Fork 639
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
Remove context lock from API VM interface #2148
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.
The changes are done in this file to ensure that json.Marshal
is called with the lock held. Previously the json-rpc logic would marshal the structs with the lock held. However, now we release the lock before passing the reply
back into the json-rpc framework.
@@ -69,11 +70,14 @@ func NewService(config Config) (*common.HTTPHandler, error) { | |||
}, "admin"); err != nil { | |||
return nil, err | |||
} | |||
return &common.HTTPHandler{Handler: newServer}, nil |
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 used to hold a WriteLock
- note: the lock was scoped to this API.
@@ -43,7 +44,7 @@ func NewService(log logging.Logger, chainManager chains.Manager, httpServer serv | |||
newServer.RegisterCodec(codec, "application/json") | |||
newServer.RegisterCodec(codec, "application/json;charset=UTF-8") | |||
|
|||
return &common.HTTPHandler{Handler: newServer}, newServer.RegisterService(ipcServer, "ipcs") |
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 used to hold a WriteLock
- note: the lock was scoped to this API.
see: #2165 |
Why this should be merged
While the
ctx.Lock
was important to add in the early days - it is heavily overused. This is the start of the effort to completely remove thectx.Lock
from thesnow.Context
.How this works
Changes
vm.CreateHandlers
andvm.CreateStaticHandlers
to no longer expose any locking options.For the most part, this PR doesn't try to reduce the scope of locking. It primarily just moves the locking from the API server into the individual services. There will be follow-up PRs to actually reduce the scope of the locking.
How this was tested
CI