Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Add fork handling to GC#2817

Merged
dlang-bot merged 1 commit intodlang:stablefrom
CyberShadow:pull-20191006-115444
Oct 7, 2019
Merged

Add fork handling to GC#2817
dlang-bot merged 1 commit intodlang:stablefrom
CyberShadow:pull-20191006-115444

Conversation

@CyberShadow
Copy link
Member

Make sure a fork does not happen while GC code is running, thus leaving it in an inconsistent state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this pointer should be reset before gcx is destroyed. BTW: as only members of gcx are affected, how about moving this and the initialization into Gcx.Dtor and initialize, respectively?

Copy link
Member Author

@CyberShadow CyberShadow Oct 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lock is in ConservativeGC, and we need to refer to it from the callbacks. Accessing ConservativeGC from Gcx seemed to me like a layering violation, but I see now there is precedent.

@CyberShadow CyberShadow force-pushed the pull-20191006-115444 branch from 63fb658 to f593df9 Compare October 6, 2019 12:53
@CyberShadow
Copy link
Member Author

I found a new bug while playing around to create a test case for this PR.

import core.stdc.stdio : printf;
import core.stdc.stdlib : exit;
import core.sys.posix.sys.types : pid_t;
import core.sys.posix.sys.wait : waitpid;
import core.sys.posix.unistd : fork;
import core.thread : Thread, getpid;

extern(C) int syscall(int, ...);
pid_t gettid() { return syscall(186/*SYS_gettid*/); }

void main()
{
	foreach (t; 0 .. 10)
		new Thread({
			auto parent = gettid();
			foreach (n; 0 .. 100)
			{
				foreach (x; 0 .. 100)
					new ubyte[x];
				auto child = fork();
				assert(child >= 0);
				if (child == 0)
				{
					child = gettid();
					printf("%d has forked.\n", child);
					foreach (x; 0 .. 100)
						new ubyte[x];
					printf("%d is exiting.\n", child);
					exit(0);
				}
				else
				{
					printf("%d waiting on %d...\n", parent, child);
					waitpid(child, null, 0);
					printf("%d done waiting on %d.\n", parent, child);
				}
			}
		}).start();
}

This program doesn't exit for me, because some threads are locked in a waitpid. The strange thing, however, is that the process they're waiting on actually has exited and is a zombie. I don't yet understand what is happening.

@CyberShadow CyberShadow force-pushed the pull-20191006-115444 branch from fdd2271 to b04d060 Compare October 6, 2019 14:02
@CyberShadow CyberShadow marked this pull request as ready for review October 6, 2019 14:03
Make sure a fork does not happen while GC code is running, thus
leaving it in an inconsistent state.

Does not yet fix Druntime's thread list after a fork, so garbage
collection inside the forked process will still fail.

Fixes issue #20271.
@CyberShadow CyberShadow force-pushed the pull-20191006-115444 branch from b04d060 to 2ed4a03 Compare October 6, 2019 14:03
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @CyberShadow!

Bugzilla references

Auto-close Bugzilla Severity Description
20271 normal Handle forking in the GC

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + druntime#2817"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Oct 6, 2019
{
if (instance)
ConservativeGC.gcLock.unlock();
}
Copy link
Member Author

@CyberShadow CyberShadow Oct 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will probably later move these out to a general Druntime fork handler which also takes care of the thread stuff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That probably won't work as the GC can be swapped out at link time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean. The atfork notifications would then be part of the abstract GC interface.

@dlang-bot dlang-bot merged commit 1a863bc into dlang:stable Oct 7, 2019
@rainers rainers mentioned this pull request Jul 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Bug Fix Include reference to corresponding bugzilla issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments