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(server-timings): lock header while inspecting/modifying Extra #220

Merged
merged 1 commit into from
May 10, 2023

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented May 10, 2023

Reported from Saturn:

fatal error: concurrent map writes

goroutine 8048914 [running]:
github.com/filecoin-project/lassie/pkg/server/http.ipfsHandler.func1.3({0x1831098, 0xc0268c0cf0})
        /github/workspace/pkg/server/http/ipfs.go:250 +0x346
github.com/filecoin-project/lassie/pkg/retriever.makeOnRetrievalEvent.func1({0x1831098?, 0xc0268c0cf0?})
        /github/workspace/pkg/retriever/retriever.go:226 +0x3ba
github.com/filecoin-project/lassie/pkg/retriever.collectResults({0x18252b0, 0xc001ee9600}, 0xc01c73f4c0, 0xc01b61dc20)
        /github/workspace/pkg/retriever/graphsyncretriever.go:242 +0x1bf
github.com/filecoin-project/lassie/pkg/retriever.(*graphsyncRetrieval).RetrieveFromAsyncCandidates(0xc0147c0500, 0xc01b61dec0)
        /github/workspace/pkg/retriever/graphsyncretriever.go:215 +0x458
github.com/filecoin-project/lassie/pkg/types.AsyncRetrievalTask.Run(...)
        /github/workspace/pkg/types/types.go:131
github.com/filecoin-project/lassie/pkg/retriever/coordinators.Race.func1.1.1()
        /github/workspace/pkg/retriever/coordinators/race.go:23 +0x90
created by github.com/filecoin-project/lassie/pkg/retriever/coordinators.Race.func1.1
        /github/workspace/pkg/retriever/coordinators/race.go:21 +0x105

Header has a lock: https://pkg.go.dev/github.com/mitchellh/[email protected]#Header and it's used for the header.NewMetric call just below this section, so this is a double-lock unfortunately.

Longer-term solution might be to go back to the state we had originally where this callback is only called on the goroutine of the caller, so there's no concurrent executions at all. I think we changed that when we added bitswap retriever and added the complexity of scheduling and racing.

@rvagg rvagg requested a review from willscott May 10, 2023 00:39
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2023

Codecov Report

Merging #220 (d98ab7b) into main (a2df0b9) will increase coverage by 0.15%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
+ Coverage   70.62%   70.78%   +0.15%     
==========================================
  Files          62       62              
  Lines        5270     5274       +4     
==========================================
+ Hits         3722     3733      +11     
+ Misses       1350     1344       -6     
+ Partials      198      197       -1     
Impacted Files Coverage Δ
pkg/server/http/ipfs.go 59.63% <100.00%> (+0.75%) ⬆️

... and 5 files with indirect coverage changes

@kylehuntsman kylehuntsman merged commit eac3f24 into main May 10, 2023
@kylehuntsman kylehuntsman deleted the rvagg/server-timings-modification-lock branch May 10, 2023 00:55
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.

3 participants