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

Revert "feat(connecteventmanager): block Connected() until accepted (#435)" and tests #444

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Aug 21, 2023

This reverts commit 7ec68c5.
This reverts commit 59a2bca.
This reverts commit 1d2f5e5.

See #436 (comment)

cc @hannahhoward @rvagg if the fix were working out for you, I think you can continue to run the previous commit while a proper fix is made.

@Jorropo Jorropo requested a review from a team as a code owner August 21, 2023 14:12
@Jorropo Jorropo enabled auto-merge (rebase) August 21, 2023 14:15
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #444 (b15f2a5) into main (db4e43a) will decrease coverage by 0.47%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #444      +/-   ##
==========================================
- Coverage   50.11%   49.64%   -0.47%     
==========================================
  Files         249      249              
  Lines       29998    29980      -18     
==========================================
- Hits        15033    14885     -148     
- Misses      13532    13659     +127     
- Partials     1433     1436       +3     
Files Changed Coverage Δ
bitswap/network/connecteventmanager.go 0.00% <0.00%> (-78.81%) ⬇️

... and 7 files with indirect coverage changes

@Jorropo Jorropo force-pushed the revert-bitswap-synchronous-connect-acks branch from 4db0886 to 222dde6 Compare August 21, 2023 14:20
…435)" and tests

This reverts commit 7ec68c5.
This reverts commit 59a2bca.
This reverts commit 1d2f5e5.
@Jorropo Jorropo force-pushed the revert-bitswap-synchronous-connect-acks branch from 222dde6 to b15f2a5 Compare August 21, 2023 14:22
@aschmahmann
Copy link
Contributor

aschmahmann commented Aug 21, 2023

LGTM, the fix was likely worse than the problem since blocking go-libp2p connections to finish processing in Bitswap can lead to misery across the stack (we've had this problem before). This will have to get fixed another way that doesn't block go-libp2p connections.

@Jorropo Jorropo merged commit 7f075b1 into main Aug 21, 2023
13 checks passed
@Jorropo Jorropo deleted the revert-bitswap-synchronous-connect-acks branch August 22, 2023 09: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.

None yet

2 participants