abci: remove lock protecting calls to the application interface#7984
abci: remove lock protecting calls to the application interface#7984mergify[bot] merged 25 commits intotendermint:masterfrom
Conversation
…o abci-add-mutex-to-clients
| } | ||
|
|
||
| func (s *SocketServer) unsafeRmConn(connID int) error { | ||
| if closer, ok := s.connsClose[connID]; ok { |
There was a problem hiding this comment.
Given that we're already under the lock here, and net.Conn implements io.Closer, do we need this separate map? It appears if we find a connection in s.conns we're already dropping it and closing it.
I see various closer functions plumbed around as parameters, but I maybe have missed where they're originating. Are these "close a related thing" someplace?
There was a problem hiding this comment.
unsafeRmConn does three things:
- cleans up the map(s)
- call the cancel function for the context that the connection handling threads are running for each connection
- closes the actual connection .
This is more confusing than it needs to be and I think I can make something work that's more simple.
|
This closes #7073 right? |
|
|
||
| #### ABCI Mutex | ||
|
|
||
| In previous versions of ABCI, Tendermint was prevented from making |
There was a problem hiding this comment.
main question for me is what can be concurrent, what needs to be sync and optimal setups. Setting everything as a rwmutex will cause issues like we have seen in the past. Some more clarification would help in migration of the sdk and other abci servers.
There was a problem hiding this comment.
I believe that this is really an application concern in terms of making sure that the application doesn't introduce some racey/non-deterministic behavior.
I think the problems we've seen in the past are "tendermint making concurrent calls to the app exercised application bugs".
There was a problem hiding this comment.
so an app can set it up in any way they want? might make sense to document this. Building an app docs need updating as this is something to be careful with .
There was a problem hiding this comment.
That's the point of this note. I've added some more text, but let me know if we should change it more.
Closes #7073
As part of the 0.36 cycle we've discussed and decided to remove the mutex in tendermint that protects the ABCI application. First, applications should be able to be responsible for their own concurrency control, and can make more fine-grained decisions about concurrent use than tendermint ever could. Second, I've observed in recent weeks as we've been making this change that the mutex wasn't applied particularly consistently in many cases (e.g. multiple "local" connections to the application had multiple locks, etc.) so this will give more consistent experiences across ABCI execution environments, and simplifies the tendermint ABCI handling code.