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

network: align duplicateFilterCount to prevent panic on 32-bit platforms #4702

Merged
merged 3 commits into from
Oct 28, 2022
Merged

network: align duplicateFilterCount to prevent panic on 32-bit platforms #4702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d13cf0f
align duplicateFilterCount to prevent panic on 32-bit platforms
cce Oct 27, 2022
d4a6de8
add to TestAtomicVariablesAlignment
cce Oct 27, 2022
b03b309
change duplicateFilterCount to uint64
cce Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion logging/telemetryspec/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ type PeerConnectionDetails struct {
// MessageDelay is the avarage relative message delay. Not being used for incoming connection.
MessageDelay int64 `json:",omitempty"`
// DuplicateFilterCount is the number of times this peer has sent us a message hash to filter that it had already sent before.
DuplicateFilterCount int64
DuplicateFilterCount uint64
}

// CatchpointGenerationEvent event
Expand Down
10 changes: 6 additions & 4 deletions network/wsPeer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,11 @@ type wsPeer struct {
// Nonce used to uniquely identify requests
requestNonce uint64

// duplicateFilterCount counts how many times the remote peer has sent us a message hash
// to filter that it had already sent before.
// this needs to be 64-bit aligned for use with atomic.AddUint64 on 32-bit platforms.
duplicateFilterCount uint64

wsPeerCore

// conn will be *websocket.Conn (except in testing)
Expand All @@ -203,9 +208,6 @@ type wsPeer struct {

incomingMsgFilter *messageFilter
outgoingMsgFilter *messageFilter
// duplicateFilterCount counts how many times the remote peer has sent us a message hash
// to filter that it had already sent before.
duplicateFilterCount int64

processed chan struct{}

Expand Down Expand Up @@ -614,7 +616,7 @@ func (wp *wsPeer) handleFilterMessage(msg IncomingMessage) {
// large message concurrently from several peers, and then sent the filter message to us after
// each large message finished transferring.
duplicateNetworkFilterReceivedTotal.Inc(nil)
atomic.AddInt64(&wp.duplicateFilterCount, 1)
atomic.AddUint64(&wp.duplicateFilterCount, 1)
}
}

Expand Down
1 change: 1 addition & 0 deletions network/wsPeer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ func TestAtomicVariablesAlignment(t *testing.T) {
require.True(t, (unsafe.Offsetof(p.requestNonce)%8) == 0)
require.True(t, (unsafe.Offsetof(p.lastPacketTime)%8) == 0)
require.True(t, (unsafe.Offsetof(p.intermittentOutgoingMessageEnqueueTime)%8) == 0)
require.True(t, (unsafe.Offsetof(p.duplicateFilterCount)%8) == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the test needs to loop through all the fields and make sure they are all fine, instead of checking just a handful?

Copy link
Contributor Author

@cce cce Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

only fields that are used by atomic.LoadInt64, atomic.StoreInt64, atomic.AddInt64, etc.. have to be here, and these are the only 4 fields in the network package that are used with these functions.

}

func TestTagCounterFiltering(t *testing.T) {
Expand Down