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

fatal error: concurrent map read and map write #95

Closed
yseto opened this issue Aug 14, 2023 · 6 comments · Fixed by #103
Closed

fatal error: concurrent map read and map write #95

yseto opened this issue Aug 14, 2023 · 6 comments · Fixed by #103
Labels
bug A broken experience Standard GitHub label Issue caused by core project dependency modules or library WIP

Comments

@yseto
Copy link
Contributor

yseto commented Aug 14, 2023

https://github.com/microsoft/kiota-abstractions-go/blob/3afc7e9554bf182dd0cb7c34020df651216133bc/api_client_builder.go#L14C1-L16

https://github.com/microsoft/kiota-abstractions-go/blob/main/serialization/parse_node_factory_registry.go#L46

I found write lock. but, need a read lock?
@baywet
Copy link
Member

baywet commented Aug 14, 2023

Thanks for reporting this.
I guess if a read happens at the same time a write is happening, it might cause that.
I'm a bit worried about the performance impact however.
Before we move to a pull request, would you mind sharing more about your application? Where does it instantiate the client? Where does it access it? Is the client a static reference?

@yseto
Copy link
Contributor Author

yseto commented Aug 18, 2023

@baywet

I haven't been able to confirm it yet, but when I investigated the crash log, it seems that the panic occurred due to the following cases occurring at the same time.

goroutines 23,25, 23 and 25 have the same content.

goroutine 23 [select]:
net/http.(*persistConn).roundTrip(0xc00002de60, 0xc001125e80)
	/usr/local/go/src/net/http/transport.go:2638 +0x994
net/http.(*Transport).roundTrip(0xc0000d3900, 0xc000b9d200)
	/usr/local/go/src/net/http/transport.go:603 +0x7fa
net/http.(*Transport).RoundTrip(0x4179f0?, 0x1bcb6e0?)
	/usr/local/go/src/net/http/roundtrip.go:17 +0x19
net/http.send(0xc000b9d200, {0x1bcb6e0, 0xc0000d3900}, {0x8?, 0x199d900?, 0x0?})
	/usr/local/go/src/net/http/client.go:252 +0x5f7
net/http.(*Client).send(0xc0002931d0, 0xc000b9d200, {0x0?, 0x0?, 0x0?})
	/usr/local/go/src/net/http/client.go:176 +0x9b
net/http.(*Client).do(0xc0002931d0, 0xc000b9d200)
	/usr/local/go/src/net/http/client.go:716 +0x8fb
net/http.(*Client).Do(0xc001125e40?, 0x0?)
	/usr/local/go/src/net/http/client.go:582 +0x19
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.transportPolicy.Do({{0x1bcb660?, 0xc0002931d0?}}, 0x4b2201?)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/pipeline.go:50 +0x36
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.(*Request).Next(0xc001125e00)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/request.go:84 +0xf8
github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime.bodyDownloadPolicy(0xc001125e00)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/runtime/policy_body_download.go:20 +0x25
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.PolicyFunc.Do(0x4b1614?, 0x1be9f70?)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/request.go:181 +0x1f
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.(*Request).Next(0xc001125dc0)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/request.go:84 +0xf8
github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime.httpHeaderPolicy(0xc001125dc0)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/runtime/policy_http_header.go:31 +0xdc
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.PolicyFunc.Do(0xc0009b18c0?, 0xc0009b1948?)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/request.go:181 +0x1f
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.(*Request).Next(0xc001125d80)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/request.go:84 +0xf8
github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime.(*logPolicy).Do(0xc000d314e8, 0xc001125d80)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/runtime/policy_logging.go:122 +0x3ad
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.(*Request).Next(0xc001125d40)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/request.go:84 +0xf8
github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime.(*retryPolicy).Do(0x0?, 0xc001125d40)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/runtime/policy_retry.go:128 +0x5b3
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.(*Request).Next(0xc001125d00)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/request.go:84 +0xf8
github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime.telemetryPolicy.Do({{0xc000a86e70?, 0x1867060?}}, 0xc001125d00)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/runtime/policy_telemetry.go:66 +0x1c5
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.(*Request).Next(0xc001125cc0)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/request.go:84 +0xf8
github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime.includeResponsePolicy(0xc001125cc0)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/runtime/policy_include_response.go:19 +0x25
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.PolicyFunc.Do(0xc000439c80?, 0xc00073f3c8?)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/request.go:181 +0x1f
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.(*Request).Next(0xc001125c80)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/request.go:84 +0xf8
github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported.Pipeline.Do({{0xc000182d00?, 0x1be99e0?, 0xc000248690?}}, 0xc001125c80)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azcore@v1.6.0/internal/exported/pipeline.go:96 +0x190
github.com/Azure/azure-sdk-for-go/sdk/azidentity.pipelineAdapter.Do({{{0xc000182d00?, 0xf8?, 0x19adae0?}}}, 0xc000b9d100)
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azidentity@v1.2.0/azidentity.go:144 +0x271
github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/internal/comm.(*Client).do(0xc0009f64d0, {0x1be9f70, 0xc001536420}, 0xc000b9d000)
	/go/pkg/mod/github.com/!azure!a!d/microsoft-authentication-library-for-go@v0.7.0/apps/internal/oauth/ops/internal/comm/comm.go:240 +0x188
github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/internal/comm.(*Client).URLFormCall(0xc00076b430?, {0x1be9f70, 0xc001536420}, {0xc00101b980, 0x58}, 0xc000439c20, {0x1729120?, 0xc0002e1440})
	/go/pkg/mod/github.com/!azure!a!d/microsoft-authentication-library-for-go@v0.7.0/apps/internal/oauth/ops/internal/comm/comm.go:209 +0x55c
github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/accesstokens.Client.doTokenResp({{_, _}, _}, {_, _}, {{{0xc0003761e8, 0x19}, {0xc000376230, 0x47}, {0x19fc376, ...}, ...}, ...}, ...)
	/go/pkg/mod/github.com/!azure!a!d/microsoft-authentication-library-for-go@v0.7.0/apps/internal/oauth/ops/accesstokens/accesstokens.go:353 +0xf2
github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/accesstokens.Client.FromClientSecret({{_, _}, _}, {_, _}, {{{0xc0003761e8, 0x19}, {0xc000376230, 0x47}, {0x19fc376, ...}, ...}, ...}, ...)
	/go/pkg/mod/github.com/!azure!a!d/microsoft-authentication-library-for-go@v0.7.0/apps/internal/oauth/ops/accesstokens/accesstokens.go:253 +0x354
github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth.(*Client).Credential(_, {_, _}, {{{0xc0003761e8, 0x19}, {0xc000376230, 0x47}, {0x19fc376, 0x5}, {0xc000955880, ...}, ...}, ...}, ...)
	/go/pkg/mod/github.com/!azure!a!d/microsoft-authentication-library-for-go@v0.7.0/apps/internal/oauth/oauth.go:124 +0x2b6
github.com/AzureAD/microsoft-authentication-library-for-go/apps/confidential.Client.AcquireTokenByCredential({{0xc001079000, {0x1beb060, 0xc001fc3b80}, {0x1bd3848, 0xc001fc3bd0}, {{{...}, {...}, {...}, {...}, 0x1, ...}, ...}, ...}, ...}, ...)
	/go/pkg/mod/github.com/!azure!a!d/microsoft-authentication-library-for-go@v0.7.0/apps/confidential/confidential.go:483 +0x145
github.com/Azure/azure-sdk-for-go/sdk/azidentity.(*ClientSecretCredential).GetToken(0xc0009f6530, {0x1be9f70, 0xc001536420}, {{0xc0009f6540, 0x1, 0x1}, {0x0, 0x0}})
	/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/azidentity@v1.2.0/client_secret_credential.go:57 +0x1bc
github.com/microsoft/kiota-authentication-azure-go.(*AzureIdentityAccessTokenProvider).GetAuthorizationToken(0xc0010790c0, {0x1be9f70, 0xc001536390}, 0xc005e5e870, 0xc0015363c0)
	/go/pkg/mod/github.com/microsoft/kiota-authentication-azure-go@v1.0.0/azure_identity_access_token_provider.go:116 +0x863
github.com/microsoft/kiota-abstractions-go/authentication.(*BaseBearerTokenAuthenticationProvider).AuthenticateRequest(0xc0009f6550, {0x1be9f70, 0xc001536390}, 0xc004a13a40, 0xc0015363c0)
	/go/pkg/mod/github.com/microsoft/kiota-abstractions-go@v1.0.0/authentication/base_bearer_token_authentication_provider.go:45 +0x299
github.com/microsoft/kiota-http-go.(*NetHttpRequestAdapter).getHttpResponseMessage(0xc001fc3d10, {0x1be9f70, 0xc001536360}, 0xc004a13a40, {0x0, 0x0}, {0x1bedb78, 0xc0026af800})
	/go/pkg/mod/github.com/microsoft/kiota-http-go@v1.0.0/nethttp_request_adapter.go:135 +0x311
github.com/microsoft/kiota-http-go.(*NetHttpRequestAdapter).Send(0xc001fc3d10, {0x1be9f70?, 0xc00443ff20?}, 0xc004a13a40, 0xc0015361b0?, 0xc000f70dd0?)
	/go/pkg/mod/github.com/microsoft/kiota-http-go@v1.0.0/nethttp_request_adapter.go:327 +0x165
github.com/microsoftgraph/msgraph-sdk-go/serviceprincipals.(*ServicePrincipalsRequestBuilder).Get(0xc0003b37d8, {0x1be9f70, 0xc00443ff20}, 0x7?)
	/go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go@v1.1.0/serviceprincipals/service_principals_request_builder.go:94 +0xf9

goroutine 12

goroutine 12 [running]:
github.com/microsoft/kiota-abstractions-go/serialization.(*ParseNodeFactoryRegistry).GetRootParseNode(0xc0002aaa40, {0xc0005f5730?, 0xc0000e01e0?}, {0xc000881000, 0x69a, 0x800})
	/go/pkg/mod/github.com/microsoft/kiota-abstractions-go@v1.0.0/serialization/parse_node_factory_registry.go:46 +0xaf
github.com/microsoft/kiota-http-go.(*NetHttpRequestAdapter).getRootParseNode(0xc001ec0e60, {0x1be9f70, 0xc0000e01e0}, 0xc000f3c1b0, {0x1bedb78, 0xc006294900})
	/go/pkg/mod/github.com/microsoft/kiota-http-go@v1.0.0/nethttp_request_adapter.go:714 +0x258
github.com/microsoft/kiota-http-go.(*NetHttpRequestAdapter).Send(0xc001ec0e60, {0x1be9f70?, 0xc003059d40?}, 0xc002c7fec0, 0xc003059fb0?, 0xc001007d40?)
	/go/pkg/mod/github.com/microsoft/kiota-http-go@v1.0.0/nethttp_request_adapter.go:350 +0x296
github.com/microsoftgraph/msgraph-sdk-go/serviceprincipals.(*ServicePrincipalsRequestBuilder).Get(0xc00023b7d8, {0x1be9f70, 0xc003059d40}, 0x7?)
	/go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go@v1.1.0/serviceprincipals/service_principals_request_builder.go:94 +0xf9

Below is the code where we are using github.com/microsoftgraph/msgraph-sdk-go.

	graphClient, err := msgraphsdk.NewGraphServiceClientWithCredentials(credential, nil)
	if err != nil {
		return fmt.Errorf("failed to retrieve service principals: %v", err)
	}
	filter := fmt.Sprintf("appId eq '%v'",clientID)
	configuration := &serviceprincipals.ServicePrincipalsRequestBuilderGetRequestConfiguration{
		QueryParameters: &serviceprincipals.ServicePrincipalsRequestBuilderGetQueryParameters{
			Filter: &filter,
		},
	}
	result, err := graphClient.ServicePrincipals().Get(ctx, configuration)

@baywet
Copy link
Member

baywet commented Aug 18, 2023

Thanks for sharing additional context.
Where does that block get called from? Is it the implementation of a backend web service? (spawning routines per incoming client call), some kind of parallel processing? etc...
Could you hoist the client creation before that parallel processing?

@yseto
Copy link
Contributor Author

yseto commented Aug 24, 2023 •
edited
Loading

I delayed to reply.

We are using a job queue. Credentials exist in the job.
Requests are issued in parallel for multiple azure accounts.
Therefore, authentication information is set on the client in parallel processing.

@baywet
Copy link
Member

baywet commented Aug 24, 2023

Thanks for the additional information. Yes in that context it makes sense that multiple concurrent client initialization might happen, I don't believe there would be a way around.

Comparing the dotnet and go implementations I noticed something, they are not identical.

the dotnet implementation uses a try add, which means it'll only add the entry if no other is present for the same key

the go implementation replaces any existing entry, no matter what.
This difference probably results in a lot of unecessary mutations of the map. Which in turns leads to concurrent write and read.

Instead of locking on read, would you mind submitting a PR that checks whether an entry is already present before adding it please?

@baywet baywet added this to Kiota Aug 24, 2023
@github-project-automation github-project-automation bot moved this to Todo in Kiota Aug 24, 2023
@baywet baywet added bug A broken experience Standard GitHub label Issue caused by core project dependency modules or library labels Aug 24, 2023
yseto added a commit to yseto/kiota-abstractions-go that referenced this issue Sep 4, 2023
@yseto
checks whether an entry is already present before adding it
74a047d 
microsoft#95
@yseto yseto mentioned this issue Sep 6, 2023
Merged
@yseto
Copy link
Contributor Author

yseto commented Sep 6, 2023

I submitted #103

would you do code review that?

baywet pushed a commit to yseto/kiota-abstractions-go that referenced this issue Sep 7, 2023
@yseto @baywet
checks whether an entry is already present before adding it
b80c833 
microsoft#95
@github-project-automation github-project-automation bot moved this from Todo to Done in Kiota Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug A broken experience Standard GitHub label Issue caused by core project dependency modules or library WIP
Projects
Kiota
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

checks whether an entry is already present before adding it yseto/kiota-abstractions-go
2 participants
@yseto @baywet