Support a GDExtension that intentionally doesn't work with Windows FreeLibrary

[31]

Describe the project you are working on

I'm working on a project where I want to share some Go (golang) code between client and server, including some existing complicated libraries written in Go. Go lets me build a .dll/.so/...

The Go and cgo toolchains are also more intuitive/familiar to me than C/C++ ones (particularly SCons), so I may choose to write a GDExtension using Go even if it would be possible to implement in C++.

Describe the problem or limitation you are having in your project

When I write my GDExtension and hit "Reload Current Project" in the Godot editor, the GDExtension fails to start. This is because when you build a DLL with Go, it doesn't support FreeLibrary: golang/go#11100. Supporting it isn't trivial and there don't appear to be plans to do so.

The sequence of events looks like this:

1. When the Godot editor on Windows starts for the first time:
   1. GDExtension::open_library copies my libgdext.windows.x86_64.dll to ~libgdext.windows.x86_64.dll.
      (Proposal: Copy GDExtension DLLs when starting the editor on Windows so the original DLL isn't locked and can be rebuilt godot-cpp#955)
   2. It loads ~libgdext.windows.x86_64.dll.
2. I press "Reload Current Project":
   1. GDExtension::close_library calls FreeLibrary on ~libgdext.windows.x86_64.dll, which succeeds but does nothing with my Go DLL.
   2. It tries to delete ~libgdext.windows.x86_64.dll, which fails because the file is still locked. It ignores the error, and we continue:
3. The new Godot editor instance starts:
   1. GDExtension::open_library tries to delete ~libgdext.windows.x86_64.dll again. It fails, but ignores the error.
   2. It tries to copy libgdext.windows.x86_64.dll to ~libgdext.windows.x86_64.dll. This fails because ~libgdext.windows.x86_64.dll is still locked, and the GDExtension fails to load in the new Godot instance.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Make the new instance kill the old instance and wait until it's terminated before attempting GDExtension initialization.

Old approach, ~1foo.dll, ~2foo.dll, ... (click to open)

If ~foo.dll can't be deleted, try ~1foo.dll, ~2foo.dll, and so on, until the file can either be deleted or doesn't already exist.

This won't change anything for normal GDExtensions: their filenames and cleanup will continue to behave the same. It only kicks in with GDExtension binaries that "stick" when opened, and lets them work with Godot editor reloads.

I don't know if anything above ~1foo.dll would actually occur. I'm not too familiar with other ways that a GDExtension might be reloaded. (It seems to me that editor reload will go between ~1foo.dll and ~foo.dll in alternative launches, because after the old process is totally exited, the old file can be deleted.)

From what I know of GDExtension, even if there is a risk of some additional garbage, I believe it would be tied to some intentional user input, so the number of garbage files would be relatively bounded over time. It would also only happen for devs (not exported games), further narrowing impact. At least, this risk is definitely acceptable on my own project to use Go DLLs. 😅

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

• Fix Windows editor reload when a GDExtension lacks FreeLibrary support godot#87165

Old approach, ~1foo.dll, ~2foo.dll, ... (click to open)

This change to GDExtension::open_library (https://github.com/godotengine/godot/blob/13a0d6e9b253654f5cc2a44f3d0b3cae10440443/core/extension/gdextension.cpp#L692-L705) implements the suggestion, although my C++ Godot API skills are probably not the best:

// Copy the file to the same directory as the original with a prefix in the name.
// This is so relative path to dependencies are satisfied.
-String copy_path = abs_path.get_base_dir().path_join("~" + abs_path.get_file());
-
-// If there's a left-over copy (possibly from a crash) then delete it first.
-if (FileAccess::exists(copy_path)) {
-	DirAccess::remove_absolute(copy_path);
-}
+String copy_path;
+int copy_index = 0;
+do {
+	String copy_str = copy_index == 0 ? "" : itos(copy_index);
+	copy_index++;
+	copy_path = abs_path.get_base_dir().path_join("_" + copy_str + abs_path.get_file());
+
+	// If there's a left-over copy (possibly from a crash) then try to delete it.
+	DirAccess::remove_absolute(copy_path);
+} while (FileAccess::exists(copy_path));

(As an aside: it would also be nice if the GDExtension methods handled their errors more consistently! It took some time adding more error handling to dig down to the first error that actually occurred and caused this series of events.)

If this enhancement will not be used often, can it be worked around with a few lines of script?

No external workaround.

Is there a reason why this should be core and not an add-on in the asset library?

No external workaround.

[dsnopek]

Thanks!

I don't understand why step 3i fails:

The new Godot editor instance starts:

1. GDExtension::open_library tries to delete ~libgdext.windows.x86_64.dll again. It fails, but ignores the error.

At this point, I think the original Godot instance should be fully shutdown, and so even if you can't FreeLibrary() the Go-based DLL, the original process having gone away should mean that the DLL is no longer locked, right?

And if that's not the case, I think trying to figure out why that is, and if we can fix it, would be better than the solution proposed here of making more temporary copies of the DLL. (My concern with making more copies is that we'd need to make sure they get cleaned up at some point.)

[31]

At this point, I think the original Godot instance should be fully shutdown, and so even if you can't FreeLibrary() the Go-based DLL, the original process having gone away should mean that the DLL is no longer locked, right?

I haven't investigated the Godot editor's reload mechanism, but I assume that it's impossible for the old Godot to completely shut down first, because then who could launch the new editor? I suspect it shuts down (close/dispose/deconstruct/etc.) as much as possible, then launches the new editor, then closes itself. Because the DLL is held open by the old process itself, this precise order matters.

If I'm right about that restart mechanism, then reload could be changed to a three part mechanism as a more general workaround for this class of problem (and maybe there are some other bugs in this class of issue that it would help with too?):

1. The old Godot launches a new Godot process that is tasked with restarting the editor after the old Godot process is gone.
2. The new process doesn't load the project (!), waits until the old process is gone, then launches Godot again with the right parameters to load the project.
3. The new Godot process loads the project, and at this point, the "middle" process still exists, but it hasn't tried to load my GDExtension, so this is ok.

(My concern with making more copies is that we'd need to make sure they get cleaned up at some point.)

FWIW, I addressed this in the issue and it doesn't concern me.

Currently Godot fails to clean up the copy when it fails in this way, so at worst, it's only as bad as it used to be. (Well, maybe more than just one file, if something unrelated also happens that triggers this behavior. But I haven't looked deeply into reloads and I'm concerned that limiting it to two files would hamper some other situations, so I'd rather not do something like that unless there's good reason.)

[31]

I haven't looked deeply into reloads

I found this PR--it seems GDExtension reloading without restarting the editor is meant to work:

• Implement reloading of GDExtensions godot#80284

This means that changes to the editor restart process wouldn't be enough to address this proposal, because the editor shouldn't have to restart. There needs to be a way to handle having multiple locked copies of the DLLs files active at the same time (like the suggestion).

---

A more direct change to the proposal to address the cleanup concern could be having an extra ~-prefix file that keeps track of the list of temp copies that have been created, so the next editor launch can delete them.

[dsnopek]

I found this PR--it seems GDExtension reloading without restarting the editor is meant to work:

• Implement reloading of GDExtensions godot#80284

This means that changes to the editor restart process wouldn't be enough to address this proposal, because the editor shouldn't have to restart. There needs to be a way to handle having multiple locked copies of the DLLs files active at the same time (like the suggestion).

Unfortunately, making multiple copies won't help in this case. If we can't FreeLibrary()/dlclose() the GDExtension, then we can't reload it, because there will be conflicting symbols when trying to load the new copy while the old copy is still loaded.

[dsnopek]

At this point, I think the original Godot instance should be fully shutdown, and so even if you can't FreeLibrary() the Go-based DLL, the original process having gone away should mean that the DLL is no longer locked, right?

I haven't investigated the Godot editor's reload mechanism, but I assume that it's impossible for the old Godot to completely shut down first, because then who could launch the new editor? I suspect it shuts down (close/dispose/deconstruct/etc.) as much as possible, then launches the new editor, then closes itself. Because the DLL is held open by the old process itself, this precise order matters.

I did a little research into this. It looks like you're correct with regard to the order, and needing to have an existing process around to start the new process. I'm not that familiar with Windows, so I was imagining we'd have something like Linux's exec()-family of functions that can replace the current process, but it looks like that's not possible on Windows.

On the Godot side, it's OS::create_instance() that actually launches the new Godot instance. So, we could conceivably have an OS_Windows::create_instance() that took the extra steps to "trampoline" the new Godot into existence after the old one has fully exited?

[31]

Unfortunate about loading it multiple times not working. That works fine for my use, I was only shooting for full support so the value of the proposal is a bit stronger. 😅

Another approach came to mind that's maybe simpler than a generalized trampoline: a new editor arg --wait-until-process-terminated <pid> (or env variable). The old instance passes its own PID to the new one, the arg delays all of the normal editor startup including GDExtension loading, so (I think) the delete works.

[dsnopek]

Another approach came to mind that's maybe simpler than a generalized trampoline: a new editor arg --wait-until-process-terminated <pid> (or env variable). The old instance passes its own PID to the new one, the arg delays all of the normal editor startup including GDExtension loading, so (I think) the delete works.

That's an interesting idea! Unless we want to implement that for every platform, we'd need to come up with a nice way to isolate it in the Windows platform code somehow. And, unfortunately, I'm not personally adept enough with Windows to come up with an implementation for that.

[31]

The old instance passes its own PID to the new one, the arg delays all of the normal editor startup

I thought about this some more, and I'm concerned about a race that could happen if the OS schedules things a certain way:

1. The old instance starts the new instance, passing it --wait-until-process-terminated <old pid>
2. The old instance stops.
3. A new, unrelated process starts and takes over the old PID. (It's free to use again as soon as the old process is gone.)
4. The new instance finally starts running, and checks to see if <old pid> is running. It is! But it's the wrong process.
5. Godot waits to start until the unrelated process is gone--which it might never be. Godot appears frozen.

I figure signaling of some kind is needed to avoid the race.

---

A very simple signal would be for the new process to actually be responsible for killing the old process. The arg would be --kill-previous-editor-pid <pid>. Godot already has an external-PID kill that can be used. (Although it needs to be enhanced to actually wait for the process to end, not just start the kill.)

I implemented this new approach at 31/godot@12c46cf. (Not ready for PR, but want to know if the approach is ok in general.)

On my machine with my simple GDExtension, it seems to work. I implemented it in a way that it should do nothing on non-Windows platforms, because it's not necessary there.

[31]

I polished it up a bit and submitted it as a PR to master at godotengine/godot#87165.

[dsnopek]

@31 I don't like the idea of killing the process. It really doesn't solve the problem you describe, that is: a new unrelated process can still start with the same PID, but now we're killing it, which seems like a worse outcome.

What if it waited for the process, but had a reasonable timeout, where it would give up waiting if it took longer than, say, 5 seconds?

[31]

(Replied at godotengine/godot#87165 (comment).)