Refactor PROJ thread context handling #76
Conversation
frewsxcv
force-pushed
the
frewsxcv-nonnull
branch
from
0b4a2ba to
c553de2
Compare
|
Bors try |
tryBuild succeeded: |
| @@ -892,7 +889,6 @@ impl Drop for Proj { | |||
| proj_area_destroy(area) | |||
| } | |||
| proj_destroy(self.c_proj); | |||
There was a problem hiding this comment.
There is a change in ordering here, right?
previously:
proj_destroy(self.c_proj);
proj_context_destroy(self.ctx);
proj_cleanup();
Now we're effectively calling:
proj_destroy(self.c_proj);
- proj_context_destroy(self.ctx);
proj_cleanup();
+ proj_context_destroy(self.ctx); // via Drop impl on ThreadContextWhich seems at odds with the comment onproj_cleanup:
// NB do NOT call until proj_destroy and proj_context_destroy have both returned:
I don't actually know the repercussions of this though.
There was a problem hiding this comment.
Wow wow great job catching this 😮
The only way I can think of to work around this is with https://doc.rust-lang.org/std/mem/struct.ManuallyDrop.html is clunky. Ugh
There was a problem hiding this comment.
Also note to future selves: think about a way to write a regression test ensuring we call these FFI calls in the right order on drop
There was a problem hiding this comment.
Would it make sense to move proj_cleanup into ThreadContext#drop?
There was a problem hiding this comment.
Maybe! Will need to think on it. Also relevant: #75
| let ctx = unsafe { std::mem::replace(&mut self.ctx, proj_context_create()) }; | ||
| Some(transform_string(ctx, definition)?) | ||
| pub fn proj(self, definition: &str) -> Option<Proj> { | ||
| Some(transform_string(self.ctx.clone(), definition)?) |
There was a problem hiding this comment.
neat - I didn't know about context_clone!
|
Closing this for now. I don't have a great solution for the issue @michaelkirk brought up, and no longer feeling confident in this strategy. |
This pull request contains a new opaque struct
ThreadContextthat contains all thread context handling.One benefit of this is the
Dropimplementations forProjandProjBuilderno longer need to handle destroying the thread context since that's now handled by theDropimplementation forThreadContext. Also,ThreadContextnow implementsClone, which gets us one step closer to implementingCloneforProj. I chose theThreadContextname because it's a little more descriptive thanContextand the PROJ docs call this concept a "thread context". I created a separate file for it because I noticedproj.rsis getting on the larger side.