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

Attempt at making JSONAPI::Consumer connection's Thread-safe #22

Closed
wants to merge 4 commits into from

Conversation

jsmestad
Copy link
Owner

Background

This is an attempt to make JSONAPI::Consumer have thread-safe connection configurations. It was originally pointed out in JsonApiClient/json_api_client#255 and mixed up what the actual ask was for.

Changes

  • Inherit connection-related attributes from a parent's superclass unless set on the child directly.
  • Add explicit tests for clearing out a relationship.

Feedback

@mltsy I am not the best at thread-safety stuff, but I made an attempt. You mind giving some feedback if I messed it up?

@jsmestad jsmestad self-assigned this Apr 16, 2018
@mltsy
Copy link

mltsy commented Apr 16, 2018

Ahhh, nice! Well, that looks... approximately right to me ;) It's probably too big of a change for me to say with confidence that it is correct just by looking at it though. I know the previously implemented thread-local approach with headers was tested and working, so if that test is still passing, then it's likely this new approach (ported from ActiveResource, I gather?) is implemented correctly! :) But if I were you I would add another test similar to the multi-thread test for headers, but vary sites/connection_options in each thread instead of headers, just to be sure it's working as expected!

@jsmestad
Copy link
Owner Author

@mltsy I noticed some funny threading issues in this code. I am going to go with #23 for now and come back to this.

I think the better move would be to go with a instance variable / shared connection system instead of trying to make class attributes thread-safe.

@mltsy
Copy link

mltsy commented Apr 17, 2018

Oh, you're right - I was not paying attention to the fact that these were all class_attributes ... that's a little more complicated. I'm not sure how to make threadsafe_attribute work alongside class_attribute (if only because I haven't looked into it, perhaps). My instinct would be to copy the logic for headers over to apply to site and connection as well. But I see you've been having problems with the headers' thread-safety as well. If you get that figured out though - just using the same kind of model for connection might work (?).

@jsmestad jsmestad closed this Apr 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants