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

Make geogram initialization less intrusive. #64

Closed
jdumas opened this issue Mar 1, 2023 · 3 comments
Closed

Make geogram initialization less intrusive. #64

jdumas opened this issue Mar 1, 2023 · 3 comments
Milestone

Comments

@jdumas
Copy link
Contributor

jdumas commented Mar 1, 2023

Currently geogram requires a call to GEO::initialize(), which is very intrusive:

  • Sets its own env variable LC_NUMERIC on Unix system
  • Register its own atexit() function.
  • Process::initialize will install new signal handlers by default
  • The initialization function cannot be called concurrently either
  • Etc.

Ideally such an initialization function should not be needed for a library integrated in a larger system. My recommendation:

  • Use a single "Context" object to register global objects that are necessary (e.g. io plugins).
  • Use Meyer’s Singleton to ensure thread-safe initialization and proper termination functions are called at program exit, without a specific atexit() function.
  • Remove all intrusive registrations by default (signals, env variables, fpe, etc.)
  • Remove the need to call GEO::initialize() altogether (e.g. have each system call their own singleton struct as needed, or rework systems to remove the need for globals altogether).
@jdumas
Copy link
Contributor Author

jdumas commented Mar 1, 2023

There are other very annoying behavior with the CmdLine args. For example, Delaunay::create() tries to retrieve CmdLine::get_arg("algo:delaunay"), but this arg is only defined if one calls import_arg_group_algo(), which is not something that GEO::intialize() seems to be doing by default. This sort of deep dependency to obscure command-line args is scattered all over the place, which leads to crashes if one does not initialize geogram carefully.

A much better approach would be to default-initialize simple parameter structs in various headers, and only tie that to actual cmd line args in Geogram's applications. I'm a big fan of CLI11's API for doing this (e.g. see this example in Lagrange).

@BrunoLevy
Copy link
Owner

  • 100% agree about most of the things.
  • about Meyer's singletons, I was using that in the past (and I'm still using that in Graphite. What I don't like is that it is sometimes difficult to know in which order the destructors of the global objects are called. I found that a plain old non-magical initialize() / terminate() pair of functions was clearer, but you are not the only one complaining about that so I should do something ...
  • what is the problem with registering functions with atexit() ? (there is a limited number of functions that one can register with atexit(), but it is still a large number I think)

@jdumas
Copy link
Contributor Author

jdumas commented Mar 2, 2023

Ideally, a library shouldn't use any global at all. I'd prefer to have to manually create a Context object, and pass it around as needed, but I recognize that having "some" globals can be useful (e.g. logging). I think there are a lot of patterns in Geogram that could be reworked to remove the need for globals altogether though. E.g. the "IO" stuff could hardcode the list of default readers/writers available, and provide a mechanism to hook up additional ones (maybe through some simple polymorphism/dispatch mechanism). The CmdLine stuff can be removed and each algorithm can use a dedicated parameter struct (optional and populated with sane default args), etc.

Regarding the atexit stuff, I'm not sure how well this works with dynamically loaded shared library (e.g. a plugin in Blender). But in any case, I'd much prefer to have the client call the terminate() function manually as they want, rather than silently registering it through atexit() (which is guaranteed to handle at least 32 functions, but not more).

@BrunoLevy BrunoLevy added this to the Contexts milestone Mar 3, 2023
BrunoLevy added a commit that referenced this issue Jul 15, 2024
Add init flags and make init less intrusive by default (fixes #64)
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

No branches or pull requests

2 participants