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.

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

ethstats: avoid concurrent write on websocket, fixes #21403 #21404

Merged
merged 3 commits into from
Aug 4, 2020

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Aug 3, 2020

See #21403 for more details. This PR adds a mutex around the writes

@holiman holiman requested a review from karalabe August 3, 2020 09:46
@holiman
Copy link
Contributor Author

holiman commented Aug 3, 2020

From https://godoc.org/github.com/gorilla/websocket#hdr-Concurrency :

Connections support one concurrent reader and one concurrent writer.

Applications are responsible for ensuring that no more than one goroutine calls the write methods (NextWriter, SetWriteDeadline, WriteMessage, WriteJSON, EnableWriteCompression, SetCompressionLevel) concurrently and that no more than one goroutine calls the read methods (NextReader, SetReadDeadline, ReadMessage, ReadJSON, SetPongHandler, SetPingHandler) concurrently.

The Close and WriteControl methods can be called concurrently with all other methods.

Looks like we need one mutex (one for writes, one for reads)

@holiman
Copy link
Contributor Author

holiman commented Aug 3, 2020

Fixed by using the same mutex for both reads and writes. The particular optimization to allow concurrent reading/writing is not important for this code.

@holiman holiman added this to the 1.9.19 milestone Aug 3, 2020
@holiman holiman requested a review from fjl August 3, 2020 17:33
Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

The locking looks a bit scary. I'd be much happier if we just made a wrapper struct for the connection that exposes ReadJSON, WriteJSON and Close methods. You could then do the locking in ReadJSON and WriteJSON.

@holiman
Copy link
Contributor Author

holiman commented Aug 4, 2020

The locking looks a bit scary. I'd be much happier if we just made a wrapper struct for the connection that exposes ReadJSON, WriteJSON and Close methods. You could then do the locking in ReadJSON and WriteJSON.

Yup, good call, will fix

@holiman
Copy link
Contributor Author

holiman commented Aug 4, 2020

Fixed, PTAL

@fjl fjl merged commit 82a9e11 into ethereum:master Aug 4, 2020
enriquefynn pushed a commit to enriquefynn/go-ethereum that referenced this pull request Mar 10, 2021
gastonponti added a commit to celo-org/celo-blockchain that referenced this pull request Apr 7, 2021
Description
Improve reliability from the ethstats module

The connectionWrapper was cherrypicked from go-ethereum upstream:
ethereum/go-ethereum@82a9e11 (ethereum/go-ethereum#21404)

Other changes
Validator injecting its version to the proxy stats chunk

Tested
Manually in a local testnet

Related issues
Fixes #1395
Fixes #1397
Backwards compatibility
Yes
oneeman pushed a commit to celo-org/celo-blockchain that referenced this pull request Apr 8, 2021
Description
Improve reliability from the ethstats module

The connectionWrapper was cherrypicked from go-ethereum upstream:
ethereum/go-ethereum@82a9e11 (ethereum/go-ethereum#21404)

Other changes
Validator injecting its version to the proxy stats chunk

Tested
Manually in a local testnet

Related issues
Fixes #1395
Fixes #1397
Backwards compatibility
Yes
oneeman pushed a commit to celo-org/celo-blockchain that referenced this pull request Apr 8, 2021
* Celostats reliability (#1487)

Description
Improve reliability from the ethstats module

The connectionWrapper was cherrypicked from go-ethereum upstream:
ethereum/go-ethereum@82a9e11 (ethereum/go-ethereum#21404)

Other changes
Validator injecting its version to the proxy stats chunk

Tested
Manually in a local testnet

Related issues
Fixes #1395
Fixes #1397
Backwards compatibility
Yes

* Skip GPM and other checks for transactions whose gas limit exceeds what's left in the block

* Change default tx pool GlobalSlots back to 4096.

The decrease to 2048 was due to inefficiencies in the tx pool which have now been fixed, so this sets it back to its original value.

* Release v1.3.0-beta.3

Co-authored-by: Gaston Ponti <pontigaston@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants