EventStore / EventStore-Client-Go

Go Client for Event Store version 20 and above.
Apache License 2.0
103 stars 25 forks source link

Fix race condition and overwriting when setting call credentials #159

Closed johnbywater closed 10 months ago

johnbywater commented 10 months ago

Fixed: Fix race condition and overwriting when setting call credentials

I adjusted the way "basic auth" call credentials are implemented, so that (1) the client configuration is not updated, (2) the client configuration is fully processed into a map of headers, (3) the connection is not given any call credentials, (4) call credentials are passed into each call as call options, (5) if credentials are provided in the options when a client method is called by user then a new credentials object is constructed and used as one of the gRPC call options, and (6) otherwise if the client been configured with credentials then those are used as one of the gRPC call options.

This means that the "global" credentials are not overwritten by credentials provided when a client method is called, and there isn't a race condition between concurrent client method calls on the overwriting of the "global" credentials.

I need this because I'm working on a project that has CI which fails on race conditions, and it failed on this race condition. Looking at it I also realised the (likely) issue of overwriting "default" client configured call credentials with credentials passed when a user calls a method (what the race condition is all about).

By the way, this is also how the Python client works, by passing in either the method credentials or otherwise the client credentials, for each call. For example here and here and here etc, 25 times.

johnbywater commented 10 months ago

@YoEight

Also, could you squash your commits, please?

I would if I could but, for whatever reason, the usual option in the Goland IDE is disabled.

However, I expect you can use the option "squash and merge" when the time comes?

YoEight commented 10 months ago

The option squash and merge is available but it doesn't add a merge commit. I don't have preference but I would rather keep in place whatever was used before.

Here's some instructions to achieve that in a terminal. If you didn't add any more commits, use the rebase command on the last 6 commits like this:

git rebase -i HEAD~6

This should open whatever is the default editor on your system. You should see your last 6 commits of you current branch. For all the commits but the first one, rename pick to fixup. For the first commit, replace pick by reword. Save and exit your editor. It should open your editor again with the message of your first commit. This time, change the message of your commit message to:

Fix race condition and overwriting when setting call credentials

Save and exit your editor. Then force-push your branch to your remote repo:

git push -f

We should see a single commit with the right message on this PR.

johnbywater commented 10 months ago

Yeah, that doesn't work either because I can't force push. It's probably the settings on your repo.

So I will close this PR, in favour of #160, which has a single commit. Sorry about that.

YoEight commented 10 months ago

The branch you use is not on the EventStore repo but yours