AXL: protect AXL's transfer file lists in multi-threading transfers

ECP-VeloC / AXL

Asynchronous Transfer Library

MIT License

2 stars 8 forks source link

AXL: protect AXL's transfer file lists in multi-threading transfers #87

Closed rhaas80 closed 3 years ago

rhaas80 commented 3 years ago

This is a continuation / transfer of https://github.com/rhaas80/AXL/pull/1 please see there for some comments.

Implementation notes:

this assumes that AXL itself is not being called from a multi-threaded code, something already (tacitly) assumed by the way the state file is handled but not explicitly spelled out
calling AXL_Config on a transfer that has already been dispatch is a race condition (since there is no lock on the individual file_list kvtrees) and not supported
calling AXL_Config for the DEBUG settings is race condition (since axl_debug is not protected but is accessed from within the worker threads) while there are already dispatched transfers and will not be fixed

tonyhutter commented 3 years ago

Sorry I've taken so long to look at this, I was out the past two weeks. So basically we're going from "AXL is somewhat, but not completely, thread safe" to telling the user "AXL is not thread safe" and removing the locking. I think that's fine, at least we're consistent now.

rhaas80 commented 3 years ago

Thank you for the review.

I have rebased onto current master, and squashed all three commits.

adammoody commented 3 years ago

@rhaas80 , while merging other PRs, I found this outstanding PR you had opened. I haven't had time to look at it in detail.

Is this still good to go?

Does this address the realloc race condition you had spotted?

rhaas80 commented 3 years ago

Well for some definition of addresses the issue. Tony summarizes the change correctly in https://github.com/ECP-VeloC/AXL/pull/87#issuecomment-736803323 and basically we are going from "we claim to everywhere be but aren't really" to "we explicitly say we aren't in the user facing function calls but otherwise we are internally" thread safe.

So it removes the bug since all access by the transfer thread is now thread safe (it keeps its own copy of the data item) while we explicitly state that calls to AXL_XXX functions from the client side must not be thread-concurrent ie the client must serialize them (though can call the functions from multiple threads).

adammoody commented 3 years ago

Awesome. Thanks, @rhaas80 . We should work to get this merged in that case. I see we've got some merge conflicts on it now.

Would you mind refreshing it?

Sorry we didn't get that merged before!

rhaas80 commented 3 years ago

Done the conflict was due to 7fa8f9e which introduced

static int bbapi_is_loaded = 0;

too close to where i removed a lock variable so get had problems merging.

adammoody commented 3 years ago

Thanks @rhaas80 !