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

fix(websocket): backport quicksink panic fix #5494

Merged
merged 3 commits into from
Jul 16, 2024

Conversation

jxs
Copy link
Member

@jxs jxs commented Jul 11, 2024

Description

Backport #5482's c19c140 to v0.52

kayabaNerve and others added 2 commits July 11, 2024 23:21
Adds the quicksink crate, which is offered under the MIT or Apache 2 license, as a module. This enables maintenance since quicksink itself has been archived for a year in a half.

Resolves libp2p#4705.

Pull-Request: libp2p#4711.
@jxs
Copy link
Member Author

jxs commented Jul 12, 2024

@lexnv since since is broken, can you test this with [patch]? If everything is accordingly I'll then release

@lexnv
Copy link
Contributor

lexnv commented Jul 15, 2024

@jxs will test this right away, thanks! Will come back with a triaging output

@lexnv
Copy link
Contributor

lexnv commented Jul 15, 2024

Thinking about it, would cherry-picking 003bb0b instead make things easier here?

I've initially created that patch fix for an easier release process, and has been in-depth tested 😄
Then we can have a release with passing CIs and tests, what do you think? @jxs @dariusc93

@jxs
Copy link
Member Author

jxs commented Jul 15, 2024

Thinking about it, would cherry-picking 003bb0b instead make things easier here?

I've initially created that patch fix for an easier release process, and has been in-depth tested 😄 Then we can have a release with passing CIs and tests, what do you think? @jxs @dariusc93

the problem with the CI doesn't relate with this PR, different CI tasks became broken due to multiple reasons that were addressed with other PR's, which could be cherry-picked into this commit.
I assumed you also tested in depth #5482 that's why you accepted the proposed patch, was that not the case?

@lexnv
Copy link
Contributor

lexnv commented Jul 16, 2024

The fix looks stable, attaching triaging:

Count      | Level      | Triage report
2224       | warn       | Notification block pinning limit reached. Unpinning block with hash = .*
26         | warn       | Report .*: .* to .*. Reason: .*. Banned, disconnecting. ( Not requested block data. Banned, disconnecting.    )
13         | warn       | Report .*: .* to .*. Reason: .*. Banned, disconnecting. ( Unsupported protocol. Banned, disconnecting.    )
9          | warn       | Report .*: .* to .*. Reason: .*. Banned, disconnecting. ( Same block request multiple times. Banned, disconnecting.    )
1          | warn       | ❌ Error while dialing .*: .*
1          | warn       | Report .*: .* to .*. Reason: .*. Banned, disconnecting. ( Grandpa: Past message. Banned, disconnecting.    )
1          | warn       | Report .*: .* to .*. Reason: .*. Banned, disconnecting. ( Grandpa: Duplicate neighbor message without grace period. Banned, disconnecting.    )

The node also looks healthy from the metrics perspective.

I was a bit scared about the libp2p CI failing, good to know its not about this backport. Indeed I tested both patches :D
Thanks a lot @jxs 🙏

@jxs
Copy link
Member Author

jxs commented Jul 16, 2024

The fix looks stable, attaching triaging:

Count      | Level      | Triage report
2224       | warn       | Notification block pinning limit reached. Unpinning block with hash = .*
26         | warn       | Report .*: .* to .*. Reason: .*. Banned, disconnecting. ( Not requested block data. Banned, disconnecting.    )
13         | warn       | Report .*: .* to .*. Reason: .*. Banned, disconnecting. ( Unsupported protocol. Banned, disconnecting.    )
9          | warn       | Report .*: .* to .*. Reason: .*. Banned, disconnecting. ( Same block request multiple times. Banned, disconnecting.    )
1          | warn       | ❌ Error while dialing .*: .*
1          | warn       | Report .*: .* to .*. Reason: .*. Banned, disconnecting. ( Grandpa: Past message. Banned, disconnecting.    )
1          | warn       | Report .*: .* to .*. Reason: .*. Banned, disconnecting. ( Grandpa: Duplicate neighbor message without grace period. Banned, disconnecting.    )

The node also looks healthy from the metrics perspective.

I was a bit scared about the libp2p CI failing, good to know its not about this backport. Indeed I tested both patches :D Thanks a lot @jxs 🙏

awesome, thanks for running the tests! released 0.42.2

@jxs jxs merged commit 3f8db40 into libp2p:v0.52 Jul 16, 2024
25 of 70 checks passed
@jxs jxs deleted the backport-websocket-fix branch July 16, 2024 21:14
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.

4 participants