jsmestad / jsonapi-consumer

Client framework for consuming JSONAPI services in Ruby
https://github.com/jsmestad/jsonapi-consumer
Apache License 2.0
94 stars 18 forks source link

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

Closed jsmestad closed 6 years ago

jsmestad commented 6 years ago

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

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?

mltsy commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 (?).