Refactor "global" state #5546

ethomson · 2020-06-05T06:47:18Z

The notion of "global" state has become somewhat confused over time; global.c handles the libraries initialization and shutdown functions, state that's global to the library and thread-local data. This is an attempt to clean this up, breaking each of these notions out into its own separate section instead of keeping them comingled and simplifying the initialization and shutdown of the library by using our existing patterns in places that we had not done that.

git_libgit2_init sets up global state for a number of subsystems (it sets up the allocator functions, does hash initialization, etc). Most of these subsystems have their init functions set up in an init_callbacks array, and their init functions register shutdown callbacks. But not all; some have initialization scattered around the git_libgit2_init machinery. Move them all into the same init/shutdown pattern:
- thread initialization
- win32 debug stack
- thread-local data (error state, etc)
- user-configurable settings (user agent string, etc)
- mwindow mutex
The "global state" (GIT_GLOBAL) is really thread-local state and contains things like the current thread's error data. Rename it accordingly, so that it's easier to reason about.
Moves the current thread's return data on Windows (for pthread_exit emulation) out of the shared TLS data structure, and managing it with the rest of the threading initialization/teardown routines.
Adds a "runtime" layer to handle the init/shutdown functions, so that we git_runtime_shutdown_register to register a shutdown handler with the runtime (instead of git__shutdown_register - the git__ name feels like an antipattern)

pks-t

I'm a huge fan of these refactorings, thanks a lot for working on them!

src/global.c

src/tlsdata.c

src/tlsdata.h

src/global.c

src/alloc.c

src/global.c

pks-t · 2020-07-12T12:55:28Z

src/settings.c

+extern size_t git_mwindow__window_size;
+extern size_t git_mwindow__mapped_limit;
+extern size_t git_indexer__max_objects;
+extern bool git_disable_pack_keep_file_checks;


I sometimes wonder if it would improve readability if those were labelled better, like e.g. calling them git_settings_mwindow_window_size. That'd make it much more obvious it's a global setting.

Anyway, definitely nothing we have to do as part of your PR.

src/tlsdata.c

src/runtime.c

src/transports/ssh.c

src/sysdir.c

src/win32/thread.c

pks-t · 2020-07-12T13:11:30Z

src/libgit2.c

 	return git_runtime_shutdown();
 }
+
+/* Library settings */


Moving stuff like version or feature information definitely makes a ton of sense to me. But I'm not sure I'm a huge fan of moving settings over, too. I kind of get why you're doing it, but if you ask me we should instead work on clearly separating settings and make them more standalone. Like clearly identifying options with git_setting_user_agent, git_setting_ssl_ciphers etc.

WDYT?

Agree with you here, and I agree that this is continuing to grow in scope. Let's tackle this in a followup PR?

src/libgit2.h

pks-t · 2020-07-12T19:11:47Z

@ethomson It's becoming quite hard to review due to the PRs growing scope. Would it be a huge burden to ask you to split up this PR into multiple ones to get those parts in more quickly? Fine with a "no" if you think the effort for this is too high.

ethomson · 2020-07-12T20:21:52Z

@ethomson It's becoming quite hard to review due to the PRs growing scope. Would it be a huge burden to ask you to split up this PR into multiple ones to get those parts in more quickly? Fine with a "no" if you think the effort for this is too high.

Sure. I split out the renaming of crtdbg into #5580.

pks-t · 2020-07-13T16:33:56Z

On Sun, Jul 12, 2020 at 01:22:45PM -0700, Edward Thomson wrote: @ethomson commented on this pull request. > @@ -48,3 +76,280 @@ int git_libgit2_shutdown(void) { return git_runtime_shutdown(); } + +/* Library settings */ Agree with you here, and I agree that this is continuing to grow in scope. Let's tackle this in a followup PR?

Fair enough. Is the PR currently in a state where it makes sense to do another review?

ethomson · 2020-07-13T19:42:59Z

Fair enough. Is the PR currently in a state where it makes sense to do
another review?

Yes I think so

Instead of treating win32 thread initialization specially in the win32 git_libgit2_init function, add a git_global_threads_init function.

Move the MSVC C runtime debugging bits into the allocator's global init function.

Move the settings global data teardown into its own separate function, instead of intermingled with the global state.

Move the mwindow mutex into the mwindow code itself, initializing it in the mwindow global initialization function instead of in the global initializer.

Ensure that we can allocate the error message buffer. In keeping with our typical policiess, we allow (small) memory leaks in the case where we're out of memory.

Our "global initialization" has accumulated some debris over the years. It was previously responsible for both running the various global initializers (that set up various subsystems) _and_ setting up the "global state", which is actually the thread-local state for things like error reporting. Separate the thread local state out into "threadstate". Use the normal subsystem initialization functions that we already have to set it up. This makes both the global initialization system and the threadstate system simpler to reason about.

We were never properly testing git_thread_exit. Do so.

We want to store a pointer to emulate `pthread_exit` on Windows. Do this within the threading infrastructure so that it could potentially be re-used outside of the context of libgit2 itself.

Provide a mechanism for system components to register for initialization and shutdown of the libgit2 runtime.

Now that we've identified that our global settings really aren't global at all, and refactored the library to match that, change global.c to libgit2.c, which is especially nice since the prefix of the functions matches the filename.

ethomson · 2020-10-11T20:37:34Z

I renamed the thread-local data that we carry around as part of our "global" state to "threadstate". Upon seeing #5602, it's clear that we should have a generic implementation of an internal TLS API so that (eg) checkout can use it and that "tlsdata" is extremely ambiguous in this regard.

ethomson · 2020-10-14T12:44:55Z

Since this is largely an internal refactoring, and I think that I addressed all of the feedback, I'm going to merge this to unblock #5602.

ethomson force-pushed the ethomson/init branch 2 times, most recently from e283d93 to e90b4dc Compare June 5, 2020 09:13

pks-t reviewed Jun 5, 2020

View reviewed changes

src/global.c Outdated Show resolved Hide resolved

src/tlsdata.c Outdated Show resolved Hide resolved

src/tlsdata.c Outdated Show resolved Hide resolved

src/tlsdata.h Outdated Show resolved Hide resolved

src/global.c Outdated Show resolved Hide resolved

ethomson force-pushed the ethomson/init branch 2 times, most recently from 8c8e473 to dcb3b86 Compare July 11, 2020 12:15

ethomson mentioned this pull request Jul 11, 2020

Friendlier getting started in the lack of git_libgit2_init #5578

Merged

ethomson force-pushed the ethomson/init branch 10 times, most recently from 8b1e1ba to f266347 Compare July 12, 2020 11:48

pks-t reviewed Jul 12, 2020

View reviewed changes

ethomson force-pushed the ethomson/init branch from 7546999 to 59ab4a8 Compare July 12, 2020 18:00

ethomson force-pushed the ethomson/init branch 2 times, most recently from 6f53beb to 6d7ab77 Compare July 12, 2020 20:18

ethomson mentioned this pull request Jul 12, 2020

msvc crtdbg -> win32 leakcheck #5580

Merged

ethomson force-pushed the ethomson/init branch 2 times, most recently from 6e41dd4 to 2381982 Compare July 12, 2020 21:15

implausible mentioned this pull request Aug 6, 2020

Build generic internal library interface for thread local storage #5602

Closed

ethomson added 2 commits October 11, 2020 14:42

init: move thread init to git_global_threads_init

fe12423

Instead of treating win32 thread initialization specially in the win32 git_libgit2_init function, add a git_global_threads_init function.

win32: teach the allocator to deal with crtdbg

521aa8c

Move the MSVC C runtime debugging bits into the allocator's global init function.

ethomson added 3 commits October 11, 2020 14:43

settings: localize global data

6554b40

Move the settings global data teardown into its own separate function, instead of intermingled with the global state.

mwindow: localize mutex

8aa69f8

Move the mwindow mutex into the mwindow code itself, initializing it in the mwindow global initialization function instead of in the global initializer.

global init: check error message buffer allocation

bc3919a

Ensure that we can allocate the error message buffer. In keeping with our typical policiess, we allow (small) memory leaks in the case where we're out of memory.

ethomson force-pushed the ethomson/init branch from 2381982 to d5deace Compare October 11, 2020 17:24

ethomson added 6 commits October 11, 2020 20:06

thread: restore the git_thread_exit tests

c40d2dc

We were never properly testing git_thread_exit. Do so.

thread: don't use the global tlsdata for thread exit

8970acb

We want to store a pointer to emulate `pthread_exit` on Windows. Do this within the threading infrastructure so that it could potentially be re-used outside of the context of libgit2 itself.

runtime: move init/shutdown into the "runtime"

e316b0d

Provide a mechanism for system components to register for initialization and shutdown of the libgit2 runtime.

Rename global.c to libgit2.c

1ec4702

Now that we've identified that our global settings really aren't global at all, and refactored the library to match that, change global.c to libgit2.c, which is especially nice since the prefix of the functions matches the filename.

Move libgit2 settings into libgit2.c

634c285

ethomson force-pushed the ethomson/init branch from d5deace to 634c285 Compare October 11, 2020 19:24

ethomson merged commit 20450cb into master Oct 14, 2020

ethomson deleted the ethomson/init branch December 11, 2020 14:32

Refactor "global" state #5546

Refactor "global" state #5546

Uh oh!

Conversation

ethomson commented Jun 5, 2020

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pks-t Jul 12, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pks-t Jul 12, 2020

Choose a reason for hiding this comment

Uh oh!

ethomson Jul 12, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pks-t commented Jul 12, 2020

Uh oh!

ethomson commented Jul 12, 2020

Uh oh!

pks-t commented Jul 13, 2020 via email

Uh oh!

ethomson commented Jul 13, 2020

Uh oh!

ethomson commented Oct 11, 2020

Uh oh!

ethomson commented Oct 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants