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

Fix crash on attaching external threads#2333

Merged
dlang-bot merged 1 commit intodlang:stablefrom
mihails-strasuns:fix-attach-collect
Oct 22, 2018
Merged

Fix crash on attaching external threads#2333
dlang-bot merged 1 commit intodlang:stablefrom
mihails-strasuns:fix-attach-collect

Conversation

@mihails-strasuns
Copy link
Contributor

Part of https://issues.dlang.org/show_bug.cgi?id=19288

Issue comes from combination of two factors:

  1. Calling thread_attachThis does new Thread as part of its
    implementation (because any thread context has to be wrapped into
    Thread class for registration in runtime).
  2. Current GC implementation is not prepared to be called from external
    thread context and will crash in several places if Thread.getThis
    returns null.

There are different possible fixes but it seems that the least intrusive
one is to simply skip collection when called from the context of
unregistered thread. Worst thing it can possibly do for any other code
not affected by the issue is to delay garbage collection a bit.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @mihails-strasuns! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#2333"

@mihails-strasuns
Copy link
Contributor Author

Also question: is there currently any testing infrastructure for mixing C/C++ and D code? I can't find one.

@Geod24
Copy link
Member

Geod24 commented Oct 18, 2018

Also question: is there currently any testing infrastructure for mixing C/C++ and D code? I can't find one.

It's being worked on. See Manu's various PRs, e.g. #2309

@jacob-carlborg
Copy link
Contributor

Also question: is there currently any testing infrastructure for mixing C/C++ and D code? I can't find one.

Do you need this for this PR? Should be possible to create a new thread with the Posix/Windows APIs to test this.

@mihails-strasuns
Copy link
Contributor Author

It will be a bit different because of presence of d_main but should expose similar behaviour. I will give it a try.

On an unrelated note - how does one check report from buildkite?

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Oct 18, 2018

On an unrelated note - how does one check report from buildkite?

For now one needs to have an account which requires to be invited by an admin, like @wilzbach.

@jacob-carlborg
Copy link
Contributor

The error is from testing Dub:

[INFO] Running /var/lib/buildkite-agent/builds/buildkite-agent-02-1/dlang/druntime/build/dlang-dub/test/interactive-remove.sh...
Package dub not found for registry at https://code.dlang.org/ (fallback ["registry at http://code.dlang.org/", "registry at https://code-mirror.dlang.io/", "registry at https://code-mirror2.dlang.io/", "registry at https://dub-registry.herokuapp.com/"]): No package candidate found for dub 0.9.20
No package dub was found matching the dependency 0.9.20
Fetching dub 0.9.21...
[ERROR] /var/lib/buildkite-agent/builds/buildkite-agent-02-1/dlang/druntime/build/dlang-dub/test/interactive-remove.sh:10 command failed
[ERROR] Script failure.

The same error has occurred in other PRs so it's not related to this PR.

@PetarKirov
Copy link
Member

@mihails-strasuns Try using this account to view the logs in Buildkite:

user: dummy@dlang.io
password: dlangrocks

Fixes https://issues.dlang.org/show_bug.cgi?id=19313

Issue comes from combination of two factors:

1) Calling `thread_attachThis` does `new Thread` as part of its
   implementation (because any thread context has to be wrapped into
   Thread class for registration in runtime).
2) Current GC implementation is not prepared to be called from external
   thread context and will crash in several places if `Thread.getThis`
   returns `null`.

There are different possible fixes but it seems that the least intrusive
one is to simply skip collection when called from the context of
unregistered thread. Worst thing it can possibly do for any other code
not affected by the issue is to delay garbage collection a bit.
@mihails-strasuns
Copy link
Contributor Author

Updated:

  • added test case which crashes before this fix
  • created dedicated bugzilla sub-issue to refer with "Fixes"

@dlang-bot dlang-bot merged commit 1f1cb92 into dlang:stable Oct 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants