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

[BREAKING CHANGE] BaseClientService isn't thread-safe #617

Merged
merged 3 commits into from
Oct 27, 2015
Merged

[BREAKING CHANGE] BaseClientService isn't thread-safe #617

merged 3 commits into from
Oct 27, 2015

Conversation

peleyal
Copy link
Collaborator

@peleyal peleyal commented Oct 21, 2015

Solve #592. Change the API of ConfigurableMessageHandler to expose an
enumerable of handlers and interceptors and NOT a list.

@peleyal
BaseClientService isn't thread-safe
41381e4 
Solve googleapis#592. Change the API of ConfigurableMessageHandler to expose an
enumerable of handlers and interceptors and NOT a list.
@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Oct 21, 2015
@peleyal peleyal mentioned this pull request Oct 21, 2015
Closed
@peleyal peleyal added this to the 1.10 milestone Oct 21, 2015
peleyal added a commit to peleyal/google-api-dotnet-client that referenced this pull request Oct 21, 2015
@peleyal
[DO NOT MERGE] Change the PCL projects to profile 92
47fa766 
1. It contains by mistake another pull request (googleapis#617)
2. Delete all WP Silverlight projects
3. Change PCL profile to 92
4. Add appveyor
@peleyal
Copy link
Collaborator Author

peleyal commented Oct 24, 2015

Ping... I'll be happy to get some feedback on this one.
Thanks!
Eyal

jtattermusch
jtattermusch reviewed Oct 24, 2015
View reviewed changes
Src/GoogleApis.Core/Apis/Http/ConfigurableMessageHandler.cs
{
get
{
lock (unsuccessfulResponseHandlersLock)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@jtattermusch
Copy link
Contributor

Can you setup you account to be recognized by google bot? (currently it says cla:no). This is probably done by signing CLA for your personal e-mail (if associated with your github account) or properly linking your github account with you corp e-mail.

@peleyal
Copy link
Collaborator Author

peleyal commented Oct 26, 2015

I signed it and changed my primary account email to google as well.

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Oct 26, 2015
@peleyal
Copy link
Collaborator Author

peleyal commented Oct 26, 2015

I just improved the comment. I kept the 3 locks (since I think it's a better approach, lock only what you need with minimized scope).

Ready for a (maybe?) final review.
Thanks Jan for the thorough review.

jtattermusch
jtattermusch reviewed Oct 26, 2015
View reviewed changes
Src/GoogleApis.Core/Apis/Http/ConfigurableMessageHandler.cs
/// <summary>Gets a list of <see cref="IHttpUnsuccessfulResponseHandler"/>.</summary>
public IList<IHttpUnsuccessfulResponseHandler> UnsuccessfulResponseHandlers
/// <summary>
/// Gets a snapshot of <see cref="IHttpUnsuccessfulResponseHandler"/>s associated with this instance's handlers.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@jtattermusch
Copy link
Contributor

Besides the nit, LGTM.

peleyal added a commit that referenced this pull request Oct 27, 2015
@peleyal
Merge pull request #617 from peleyal/master
9c6c193 
BaseClientService isn't thread-safe
@peleyal peleyal merged commit 9c6c193 into googleapis:master Oct 27, 2015
@peleyal peleyal changed the title BaseClientService isn't thread-safe [BREAKING CHANGE] BaseClientService isn't thread-safe Nov 9, 2015
peleyal added a commit to peleyal/google-api-dotnet-client that referenced this pull request Nov 14, 2015
@peleyal
googleapis#592 unbreaking change to ConfigurableMessageHandler
184cebd 
In googleapis#617, I presented a new breaking change to the library. With this
change we are going to avoid that, by adding back the list getter, which
is OBSOLETE now.
@peleyal peleyal mentioned this pull request Nov 14, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
1.10
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@peleyal @googlebot @jtattermusch