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

Atomic updates for the refcount #211

Closed
wants to merge 4 commits into from
Closed

Atomic updates for the refcount #211

wants to merge 4 commits into from

Conversation

EmielBruijntjes
Copy link
Contributor

I've modified the json_object_get() and json_object_put() functions to use atomic updates of the refcount. This allows json objects to be safely moved and copied between threads.

It uses the atomic builtin functions of GCC (see https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html). These functions are also already used inside the linkhash.c file of this json-c library, so I think that it is not much of a problem that they will be used inside json_object.c too.

@hawicz
Copy link
Member

hawicz commented Nov 30, 2015

ugh, no. If you want to do something like this, please do not litter the code with a bunch of ifdefs. Instead, define a macro (or inline function) in a header file to keep the compiler-specific cruft isolated.
If you clean up the existing usage in linkhash.c, that would be appreciated as well.

@EmielBruijntjes
Copy link
Contributor Author

Wow, that is such a nice and great response - given the fact that I not only found but also immediately solved a real bug, and did also my very best to keep the coding style as close to the rest of the json-c library as possible. I inspected the code to find a place in which atomic functions were already used, and I implemented it in exactly the same way. In fact, I almost copied that #ifdef block from the json-c source. Both the json_object.c and the linkhash.c files already are full of #ifdef statements. Considering all this, I find it strange that you respond to my change with "ugh".

My change basically comes down to two simple lines of code, it solves a genuine bug (refcounts in json objects are not thread safe) and it will probably take you about 1 minute and 20 seconds to convert these two lines into a format that you appreciate. The alternative is that we now start a discussion that will take many hours of both our lives before we have these two lines of code exactly formatted he way you like it.

@hawicz
Copy link
Member

hawicz commented Dec 1, 2015

Thanks for considering the existing code, and submitting a fix for a problem, but the fact is that what's already present is by no means perfect and can't always be used as an example without considering the change on its own merits. Also, there's a bit of a difference between things like the REFCOUNT_DEBUG ifdefs in json_object.c, that serve to conditionalize a "feature", and those that work around platform inconsistencies, so IMO it's not valid to use that as an excuse for adding more ifdefs.
I suggested a reasonable alternative, and you're free to re-do your change or not, as you see fit, but do not presume to have a claim on the time that I spend on json-c. I volunteer my time, and that "1 minute and 20 second" that you so flippantly claim it would take to adjust your changes is time that I could just as happily spend doing something else.

@EmielBruijntjes
Copy link
Contributor Author

I have posted it as issue instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants