PelicanPlatform / xrootd-s3-http

An XRootD plugin that allows Pelican to interface with s3/http server backends
Apache License 2.0
2 stars 6 forks source link

Switch static variable to thread_local to fix underflow bug #33

Closed jhiemstrawisc closed 6 months ago

jhiemstrawisc commented 7 months ago

Multiple threads appear to be running through read_callback with their own data, and they're fighting over the sentSoFar variable. Switching it from static to thread_local makes sure each thread has its own copy to use.

I understand less about how XRootD manages threads, but making this switch appears to fix what was otherwise a guaranteed crash.

Closes #32

bbockelm commented 6 months ago

@jhiemstrawisc and I discussed this a bit in person. While this PR's one-liner would fix the issue at hand, we noted that it still relies on the the callback to be invoked until completion to reset the thread-local sentSoFar back to 0 -- if there's a failure mid-transfer, then the variable will stay where it left off.

A better solution is to create a small struct that keeps both the buffer and the current offset in the buffer, then have that live on the heap in the function invoking curl_easy_perform. This way, all the transfer-related state is in the pointer passed to the callback instead of having a mixture of state in a thread-local and the provided reference. Since that's going to still be a minimal change -- probably 5-10 lines -- it seems prudent to do that small change instead of two small changes.

jhiemstrawisc commented 6 months ago

@bbockelm This is important for pelican 7.8 (and should be patched into 7.7), so I'm going to merge without waiting for a second review. Testing PUTS works locally and I think I implemented things the way you outlined, but if you come back and something doesn't look right I'm happy to update.