simplify and add DoNotWaitForStreamingResponses #75

Jorropo · 2023-07-19T01:47:50Z

No description provided.

…s waitgroup

I removed the search value canceled context test since it was racy, it was racing the errCh. The new code waits for one worker to not error or all of them to error before resolving instead of leaking goroutines.

Jorropo · 2023-07-19T01:52:02Z

hugo@Radus ~/k/go-libp2p-routing-helpers (refactor)> go mod tidy && git status
Sur la branche refactor
Votre branche est à jour avec 'origin/refactor'.

rien à valider, la copie de travail est propre

go.sum is good idk goreleaser is confused

aschmahmann

No code bugs identified here, mostly just concerns about the complexity and clarity of what's going on.

The potential bugs are associated with whether this design will really give us what we need in kubo (e.g. ipfs/kubo#10020 (comment)). In particular the non-generic parallel routers were pretty specific but were handling edge cases that compparallel is having problems with. We can deal with that via leveraging the composability better, if the code in the consumer (e.g. kubo) ends up looking reasonable with this abstraction then 👍 otherwise it might need to change a bit to accommodate.

compsequential.go

aschmahmann · 2023-07-21T18:42:02Z

go.mod

@@ -3,6 +3,7 @@ module github.com/libp2p/go-libp2p-routing-helpers
 go 1.19

 require (
+	github.com/Jorropo/jsync v1.0.1


What's the value of using jsync here? The commit that introduces this claims it's a performance improvement due to not needing a goroutine that might be short lived, but AFAIK this code isn't really in the critical path anywhere which means it's just another abstraction reviewers have to keep in mind.

Imo it makes the code more clear, easy as when it reaches 0 some code executes.
It's also limited to internal implementation and isn't exposed to consumers in anyway, jsync just depends on the std so I think it's fine as-is.

compparallel_test.go

compparallel.go

aschmahmann · 2023-07-21T20:27:46Z

compparallel.go

+						errorsLk.Lock()
+						if outCh == nil {
+							outCh = make(chan T)
+							errors = nil


does this do anything other than let the GC collect a few bytes earlier?

it's used in the error handling:

if errors == nil { return } errs = append(errs, err)

if errors is nil then it understand something else already had a sucessfull and it does not need to be appending.

aschmahmann · 2023-07-21T20:30:41Z

compparallel.go

-	errCh := make(chan error)
-	var wg sync.WaitGroup
+	var ready sync.Mutex
+	ready.Lock()


I think more explanation around the ready lock seems reasonable. Readers can follow the code, but with async code like this it's easy for anyone who comes next to miss something.

aschmahmann · 2023-07-21T20:32:45Z

compparallel.go

+					var minusOne uint64
+					minusOne--


Why are we doing this with uints instead of ints?

Because it can't be negative.

github-actions · 2023-07-25T17:11:01Z

Suggested version: v0.7.1

Comparing to: v0.7.0 (diff)

Changes in go.mod file(s):

(empty)

gorelease says:

# diagnostics
go.sum: one or more sums are missing. Run 'go mod tidy' to add missing sums.

# summary
Suggested version: v0.7.1

gocompat says:

Your branch is up to date with 'origin/master'.

Cutting a Release (and modifying non-markdown files)

This PR is modifying both version.json and non-markdown files.
The Release Checker is not able to analyse files that are not checked in to master. This might cause the above analysis to be inaccurate.
Please consider performing all the code changes in a separate PR before cutting the release.

Automatically created GitHub Release

A draft GitHub Release has been created.
It is going to be published when this PR is merged.
You can modify its' body to include any release notes you wish to include with the release.

Jorropo added 4 commits July 19, 2023 03:46

perf: remove goroutine that just waits before closing with a synchrou…

b9b0d9b

…s waitgroup

fix: make the ProvideMany docs clearer

3d204a1

fix: correctly handle errors in compparallel

7a4be42

I removed the search value canceled context test since it was racy, it was racing the errCh. The new code waits for one worker to not error or all of them to error before resolving instead of leaking goroutines.

chore: cleanup error handling in compparallel

88c9d0d

Jorropo force-pushed the refactor branch from 5940a40 to 79c419f Compare July 19, 2023 01:49

Jorropo requested a review from aschmahmann July 19, 2023 01:50

feat: add DoNotWaitForStreamingResponses to ParallelRouter

4c7bd7c

Jorropo force-pushed the refactor branch from 79c419f to fd78e08 Compare July 19, 2023 02:21

Jorropo mentioned this pull request Jul 19, 2023

fix: mark all routers DoNotWaitForSearchValue ipfs/kubo#10020

Merged

Jorropo self-assigned this Jul 19, 2023

aschmahmann requested changes Jul 21, 2023

View reviewed changes

Jorropo force-pushed the refactor branch 2 times, most recently from 850d18a to 95ff051 Compare July 25, 2023 17:00

Jorropo added 3 commits July 25, 2023 19:09

chore: rename DoNotWaitForStreamingResponses to DoNotWaitForSearchValue

a656061

fix: for comparallel never return nil channel for FindProvidersAsync

53808c6

chore: release v0.7.1

64ddddb

Jorropo force-pushed the refactor branch from 95ff051 to 64ddddb Compare July 25, 2023 17:09

Jorropo merged commit 2d62ab8 into master Jul 25, 2023
20 checks passed

Jorropo deleted the refactor branch July 25, 2023 17:21

BigLep mentioned this pull request Aug 3, 2023

Release 0.22 ipfs/kubo#9911

Closed

aschmahmann mentioned this pull request Nov 8, 2023

fix: routing findprovs hangs with no records ipfs/kubo#10206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplify and add DoNotWaitForStreamingResponses #75

simplify and add DoNotWaitForStreamingResponses #75

Jorropo commented Jul 19, 2023

Jorropo commented Jul 19, 2023 •

edited

Loading

aschmahmann left a comment •

edited

Loading

aschmahmann Jul 21, 2023

Jorropo Jul 22, 2023 •

edited

Loading

aschmahmann Jul 21, 2023

Jorropo Jul 24, 2023 •

edited

Loading

aschmahmann Jul 21, 2023

aschmahmann Jul 21, 2023

Jorropo Jul 24, 2023 •

edited

Loading

github-actions bot commented Jul 25, 2023

simplify and add DoNotWaitForStreamingResponses #75

simplify and add DoNotWaitForStreamingResponses #75

Conversation

Jorropo commented Jul 19, 2023

Jorropo commented Jul 19, 2023 • edited Loading

aschmahmann left a comment • edited Loading

Choose a reason for hiding this comment

aschmahmann Jul 21, 2023

Choose a reason for hiding this comment

Jorropo Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

aschmahmann Jul 21, 2023

Choose a reason for hiding this comment

Jorropo Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

aschmahmann Jul 21, 2023

Choose a reason for hiding this comment

aschmahmann Jul 21, 2023

Choose a reason for hiding this comment

Jorropo Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jul 25, 2023

Cutting a Release (and modifying non-markdown files)

Automatically created GitHub Release

Jorropo commented Jul 19, 2023 •

edited

Loading

aschmahmann left a comment •

edited

Loading

Jorropo Jul 22, 2023 •

edited

Loading

Jorropo Jul 24, 2023 •

edited

Loading

Jorropo Jul 24, 2023 •

edited

Loading