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

Interoperability issues with dependencies using Objective-C in Cluster Mode on macOS 10.13 High Sierra #1421

Closed
ticky opened this issue Sep 26, 2017 · 54 comments

Comments

Projects
None yet
9 participants
@ticky
Copy link

commented Sep 26, 2017

Steps to reproduce

  1. Install macOS 10.13 High Sierra

  2. Freshly install Ruby and Puma

  3. Execute an application which uses the pg gem, does not use preload_app!, and uses Puma in cluster mode

Expected behavior

Puma should work and execute jobs as expected

Actual behavior

Puma fails, repeatedly outputting these errors from worker processes:

objc[81924]: +[__NSPlaceholderDictionary initialize] may have been in progress in another thread when fork() was called.
objc[81924]: +[__NSPlaceholderDictionary initialize] may have been in progress in another thread when fork() was called. We cannot safely call it or ignore it in the fork() child process. Crashing instead. Set a breakpoint on objc_initializeAfterForkError to debug.

System configuration

Ruby version: 2.4.2
Rails version: 5.1.3
Puma version: 3.10.0

macOS High Sierra changes the behaviour of the fork syscall such that calls to Objective-C APIs in forked processes are treated as errors.

It appears some part of Puma’s use of Ruby violates this, which means Puma doesn’t work correctly in cluster mode on 10.13.

A temporary workaround is to apply the OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES environment variable to the parent puma process, which will then trigger the behaviour observable in macOS versions 10.12 and earlier.

EDIT: Puma uses fork in cluster mode, and no special consideration is given to what runs within the fork. It is possible to depend upon a gem, or dynamically load a shared object, which in turn links to an Objective-C framework. This linkage will implicitly call an Objective-C initialize method as it sets up the Objective-C runtime for the first time, and if this call occurs within a fork, the program will be killed, and the above error message printed.

A less heavy-handed workaround is to use a before_fork callback like this:

# Work around macOS 10.13 and later being very picky about
# `fork` usage and interactions with Objective-C code
# see: <https://github.com/puma/puma/issues/1421>
if /darwin/ =~ RUBY_PLATFORM
  before_fork do
    require 'fiddle'
    # Dynamically load Foundation.framework, ~implicitly~ initialising
    # the Objective-C runtime before any forking happens in Puma
    Fiddle.dlopen '/System/Library/Frameworks/Foundation.framework/Foundation'
  end
end

This will implicitly initialise the Objective-C framework within the Puma process, allowing forked children to use other Objective-C frameworks.

@nateberkopec nateberkopec added the bug label Sep 26, 2017

@nateberkopec

This comment has been minimized.

Copy link
Member

commented Sep 26, 2017

@nateberkopec

This comment has been minimized.

Copy link
Member

commented Sep 26, 2017

I'm with Eric. I have no idea how ObjC factors into this.

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 26, 2017

ObjC factors in because if anything within the forked process links against ObjC APIs, it will be unusable from within Puma! :)

I’m doing some more debugging to determine what exactly is causing this.

@mperham

This comment has been minimized.

Copy link

commented Sep 26, 2017

Oh crap. Subscribe me, github!

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 26, 2017

Having trouble doing more debugging, as the child exits as soon as it encounters this error, so attaching rbtrace seems difficult if not impossible.

@boazsegev

This comment has been minimized.

Copy link

commented Sep 27, 2017

Sorry to butt in, but do you know if this effects iodine as well?

I can't install High Sierra since many of the professional applications I use daily don't support it just yet (I'm also a musician).

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 27, 2017

Anything that forks and runs user-specified code within that fork without using exec may be affected by this.

It is unclear exactly where a solution will need to be implemented.

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 27, 2017

I’ve managed to track down what appears to be the source of this error in the application I’m working with, it’s loading pg which has a native extension linked against Postgres’ libpq.5 which in turn is linked against Kerberos.framework and LDAP.framework.

It seems unlikely that we can get away with our workers not being able to talk to our database on macOS… 🤔

Again, this is not that you have to actively be using any features provided by these Objective-C frameworks, their inclusion in a thing you link against is enough to trigger this error. We do not use the Kerberos nor LDAP features of Postgres in our application, and yet here we have this error.

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 27, 2017

Apparently this thread has ended up on reddit where it seems to be being misunderstood.

Regarding this comment:

I'm trying to come up with a summary of the issue; please assist:

  1. macOS includes an Objective-C runtime
  2. macOS includes an OS framework based on the Objective-C runtime
  3. Objective-C classes defined by the OS framework are fork-unsafe, meaning that fork() callers should not execute code before calling exec()
  4. macOS 10.13 includes a macOS 10.13 SDK (presumably based on the OS framework but not responsible for defining the OS framework's classes)
  5. macOS applications built with the macOS 10.13 SDK are able to use new SDK features in order to execute Objective-C code between fork() and exec(), therefore achieving the fork-safe property when used correctly
  6. There remains a tension between fork-safety and thread-safety, due to the structure of the locks held by the Objective-C runtime
  7. As such, there are restrictions in the mac OS 10.13 SDK about the nature of calls to +initialize between calling fork() and exec()
  8. Thus, the new capabilities in the mac OS 10.13 SDK for calling code safely between fork() and exec() are guarded by protections which enforce the limitations to +initialize in order to maintain fork-safe and thread-safe properties

What is the connection to Ruby on macOS 10.13? I'm not very familiar with this platform, but it would appear that the affected Ruby libs on macOS are making use of the SDK, calling fork(), and running into the new behavior.

The issue is that code within Ruby can call fork (as Puma does), and loading dynamic libraries from Ruby or a Ruby native extension can cause an Objective-C class to be +initialized, triggering this error message and a swift exit.

@boazsegev

This comment has been minimized.

Copy link

commented Sep 27, 2017

@ticky ,

I'm happy you're managing to track the issue down and I'm thankful for the added information.

However, reading the information it wasn't clear to me if pg might be producing the error independently of the server (does pg use fork?) and whether or not pg's initialization could be handled pre-forking (perhaps avoiding the issue)?

  1. Does the error occur also in single process mode (in puma and iodine, add the -w 1 to the command line)?

  2. Could you initialize thepg database within the config.ru and see if pre-fork initialization might work around the issue? (remember to re-establish the DB connections, so they aren't shared acrid processes)...?

Just my 2¢, I don't know if any of it helps.

P.S.

You posted the issue summery as I was writing this and I wondered if, perhaps, some non Objective-C function calls are implemented using the Objective-C framework under the hood..?

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 27, 2017

wasn't clear to me if pg might be producing the error independently of the server (does pg use fork?)

I don’t believe pg uses fork itself. These errors are produced by workers immediately after spinning up. It appears to occur entirely within the initialisation of our application's dependencies.

whether or not pg's initialization could be handled pre-forking

This is the best hypothesis I now have. It looks like it might work if those things are all loaded before forking, however, I am not familiar enough with the way our system is set up to know how the should work!

You posted the issue summery as I was writing this and I wondered if, perhaps, some non Objective-C function calls are implemented using the Objective-C framework under the hood..?

I don’t believe it’s that they are intentionally calling out to any particular Obj-C method, but merely linking to their Obj-C frameworks which requires the Obj-C initialisation to occur!

@mistydemeo

This comment has been minimized.

Copy link

commented Sep 27, 2017

However, reading the information it wasn't clear to me if pg might be producing the error independently of the server (does pg use fork?) and whether or not pg's initialization could be handled pre-forking (perhaps avoiding the issue)?

pg doesn't use fork, but the issue isn't active initialization by pg so much as how the ObjC runtime startup is occurring. In this context since the program itself isn't written in ObjC and doesn't directly have any ObjC linkage, the ObjC runtime only enters the equation when a dlopen call (via Ruby's dln_load) loads a shared object which has ObjC linkage. As you can see in this truncated backtrace from an aborted Ruby process the dynamic loader is initializing the ObjC runtime automatically, which causes CoreFoundation to initialize an object. That triggers the kill.

* thread #1, stop reason = signal SIGSTOP
  * frame #0: 0x00007fff53bde336 libsystem_kernel.dylib`__abort_with_payload + 10
    frame #1: 0x00007fff53bd8e00 libsystem_kernel.dylib`abort_with_payload_wrapper_internal + 89
    frame #2: 0x00007fff53bd8da7 libsystem_kernel.dylib`abort_with_reason + 22
    frame #3: 0x00007fff52ea2962 libobjc.A.dylib`_objc_fatalv(unsigned long long, unsigned long long, char const*, __va_list_tag*) + 108
    frame #4: 0x00007fff52ea2814 libobjc.A.dylib`_objc_fatal(char const*, ...) + 135
    frame #5: 0x00007fff52ea343b libobjc.A.dylib`performForkChildInitialize(objc_class*, objc_class*) + 341
    frame #6: 0x00007fff52e93e8f libobjc.A.dylib`lookUpImpOrForward + 228
    frame #7: 0x00007fff52e93914 libobjc.A.dylib`_objc_msgSend_uncached + 68
    frame #8: 0x00007fff52e97145 libobjc.A.dylib`+[NSObject new] + 86
    frame #9: 0x00007fff2e5bb025 Foundation`-[NSThread init] + 61
    frame #10: 0x00007fff2e5bf0f5 Foundation`____NSThreads_block_invoke + 64
    frame #11: 0x00007fff53a55f64 libdispatch.dylib`_dispatch_client_callout + 8
    frame #12: 0x00007fff53a55f17 libdispatch.dylib`dispatch_once_f + 41
    frame #13: 0x00007fff2e5bafe3 Foundation`_NSThreadGet0 + 325
    frame #14: 0x00007fff2e5ba7ef Foundation`_NSInitializePlatform + 341
    frame #15: 0x00007fff52e958df libobjc.A.dylib`call_load_methods + 239
    frame #16: 0x00007fff52e92c16 libobjc.A.dylib`load_images + 70
    frame #17: 0x000000011125d198 dyld`dyld::notifySingle(dyld_image_states, ImageLoader const*, ImageLoader::InitializerTimingList*) + 407
    frame #18: 0x000000011126d15b dyld`ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&, unsigned int, char const*, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) + 309
    frame #19: 0x000000011126d103 dyld`ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&, unsigned int, char const*, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) + 221
    frame #20: 0x000000011126d103 dyld`ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&, unsigned int, char const*, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) + 221
    frame #21: 0x000000011126d103 dyld`ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&, unsigned int, char const*, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) + 221
    frame #22: 0x000000011126d103 dyld`ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&, unsigned int, char const*, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) + 221
    frame #23: 0x000000011126c2a6 dyld`ImageLoader::processInitializers(ImageLoader::LinkContext const&, unsigned int, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) + 134
    frame #24: 0x000000011126c33a dyld`ImageLoader::runInitializers(ImageLoader::LinkContext const&, ImageLoader::InitializerTimingList&) + 74
    frame #25: 0x00000001112603e5 dyld`dyld::runInitializers(ImageLoader*) + 82
    frame #26: 0x0000000111269002 dyld`dlopen + 527
    frame #27: 0x00007fff53a90eb6 libdyld.dylib`dlopen + 86
    frame #28: 0x0000000102d0ce3f ruby`dln_load + 175
@ticky

This comment has been minimized.

Copy link
Author

commented Sep 27, 2017

in single process mode

I had tried to activate single-process mode earlier but not had any luck. Turns out you need to remove the workers configuration statements altogether!

Running in single mode (confirmed with Puma starting in single mode… output!) works as expected with no crashes or errors.

Thus, it’s the fork by Puma (or whatever else is using it) followed by Objective-C initialize calls within pg which is causing the ruckus. 😄

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 27, 2017

A bit of an update - this behaviour also doesn’t cause a crash if preload_app! is set to true. I can’t seem to find the implementation of this feature, though?

@mistydemeo

This comment has been minimized.

Copy link

commented Sep 27, 2017

I have a Unicorn app, rather than a Puma app, but something that's working for me is to explicitly require a gem with ObjC linkage within a before_fork block. That ensures that the ObjC runtime is loaded and initialized before the fork occurs, and it's a bit lighter-weight than preload_app!.

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 27, 2017

@mistydemeo’s onto something! I just tried adding require 'pg' to our Puma before_fork, and the errors have ceased! Thanks so much!

I wonder if there’s a less strange-looking way to achieve this than loading a specific dependency we know to link ObjC. 🤔

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 27, 2017

Okay, this doesn’t look much less strange but it is, at least, only relying on side effects it’s explicitly calling upon!

# Work around macOS 10.13 and later being very picky about
# `fork` usage and interactions with Objective-C code
# see: <https://github.com/puma/puma/issues/1421>
if /darwin/ =~ RUBY_PLATFORM
  before_fork do
    require 'fiddle'
    # Dynamically load Foundation.framework, ~implicitly~ initialising
    # the Objective-C runtime before any forking happens in Puma
    Fiddle.dlopen '/System/Library/Frameworks/Foundation.framework/Foundation'
  end
end

As Misty noted, this ensures the the Objective-C runtime has been initialised (a side effect instigated by the dynamic loader itself) before any fork happens, which means once pg is required and loads the Kerberos and LDAP frameworks (which in turn rely on Foundation), they do not need to perform that implicit initialize.

Whether this is a workaround which makes sense to include in Puma is definitely debatable, but it appears that many things which make use of this sort of fork model will need to consider it, and perhaps also document it.

Note also that I did not guard this to only occur on 10.13, just macOS in general, as this should work fine on other versions, and in fact be more “correct” behaviour by its definition.

@ticky ticky changed the title Cluster mode doesn't work on macOS 10.13 High Sierra Interoperability issues with dependencies using Objective-C in Cluster Mode on macOS 10.13 High Sierra Sep 27, 2017

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 27, 2017

Updated the initial post with the updated details of this issue!

@nateberkopec

This comment has been minimized.

Copy link
Member

commented Sep 27, 2017

Great work @ticky! I won't be able to take a closer look at this until next week - holding off on my own High Sierra upgrade until I am back home and can make some backups first.

@tenderlove

This comment has been minimized.

Copy link

commented Sep 27, 2017

According to this article, setting OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES environment variable should work too. It's really not clear how to fix this correctly (besides not lazily loading things I guess).

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 27, 2017

@tenderlove yep, I noted that in my initial post ☺️ but it’s a brute-force workaround (i.e. it may have other consequences), and doesn’t solve the fact that it may not even be possible in future OS versions!

@boazsegev

This comment has been minimized.

Copy link

commented Sep 28, 2017

doesn’t solve the fact that it may not even be possible in future OS versions

It would be possible to have a quite exception instead of the before_fork - fork could be assumed, in the interest of C extensions such as passenger and iodine, where before_fork callbacks might not get invoked due to a direct system call (rather than Kernel.fork)...

Maybe something like:

# Work around macOS 10.13 and later being very picky about
# `fork` usage and interactions with Objective-C code
# see: <https://github.com/puma/puma/issues/1421>
if /darwin/ =~ RUBY_PLATFORM
  begin
    require 'fiddle'
    # Dynamically load Foundation.framework, ~implicitly~ initialising
    # the Objective-C runtime before any forking happens in Puma
    Fiddle.dlopen '/System/Library/Frameworks/Foundation.framework/Foundation'
  rescue Exception => _e
	
  end
end

it may have other consequences

Since fiddle is part of the standard library, it seems safe enough.

However, the _NS* bindings might be another issue (introducing name space concerns and possible conflicts), which make this a very valid warning.

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 28, 2017

@boazsegev my last message was regarding the use of the environment variable (OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES) - my Ruby-based workaround will likely remain viable. ☺️

As @mistydemeo suggested, it works in Unicorn too, and it likely works in any other thing which is using fork in a similar way, and running arbitrary Ruby code within.

I would suggest that having an option to implicitly load Foundation would make some sense, however, I would be reluctant to suggest having it always implicitly do this for all users, as it feels very heavy-handed. It’s unfortunate that we can’t catch these kinds of errors in our code (the process is killed instantly), or I’d suggest adding our own message about it!

It’s also worth noting again that this behaviour doesn’t trigger this error case outside of cluster mode, or if the application is preloaded.

@boazsegev

This comment has been minimized.

Copy link

commented Sep 28, 2017

@ticky , you're probably right about it being heavy-handed, but...

it likely works in any other thing which is using fork in a similar way...

I don't think it does. Especially since before_fork is puma's DSL and not a general Ruby feature.

Other C extensions (such as passenger and iodine) might use a different DSL before they call the system call directly (not using Ruby's fork), assuming any required actions were directly performed (rather than placed in a callback).

I might be over thinking this, since the issue was opened here, but the discussion feels as if it's more than about a Puma specific solution (it includes Unicorn, Sidekiq etc').

Just my 2¢, of course.

@ticky

This comment has been minimized.

Copy link
Author

commented Sep 28, 2017

I don’t mean the before_fork call specifically, just that you’ll need some mechanism to do the dlopen call before you fork. Indeed, its use will depend on what you’re using, and could simply be to put it in whatever calls out to to Passenger or Iodine - it merely has to be done within the same process.

You are right, though, that the issue is bigger than Puma. It’s likely that Puma, Unicorn, Passenger and Iodine will all need to consider the implications of this macOS change. Unicorn’s maintainer, though, seems to have already expressed a disinterest in doing so!

Either that or it requires some sort of structural change in Ruby (though that seems rather unlikely to fly)

FooBarWidget added a commit to phusion/passenger that referenced this issue Oct 9, 2017

Ruby wrappers: load Foundation.framework during startup on macOS
Apple added an assertion in 10.13 that prevents anything Obj-C related
from occuring between fork and exec. This workaround prevents the assertion.

See:
- http://www.sealiesoftware.com/blog/archive/2017/6/5/Objective-C_and_fork_in_macOS_1013.html
- puma/puma#1421
@FooBarWidget

This comment has been minimized.

Copy link

commented Oct 11, 2017

It looks like the dlopen workaround is the best solution in terms of fewest side-effects. OBJC_DISABLE_INITIALIZE_FORK_SAFETY is too broad and affects child processes.

Regarding @ticky's comment about structural changes in Ruby: I think this goes beyond Ruby itself. Forking is fundamentally problematic when there is multithreading involved. We can make it work, and in the context of Ruby app servers it usually works well so far, but that's in spite of a lack of real solutions.

To guarantee that forking is safe, the application must not be running any threads at the point of fork (or the other threads must only be executing async-signal-safe code). MRI can guarantee this about its own code to some degree, but third party native libraries may spawn their own threads. So any gems that the user pulls in should not spawn threads until the app server has forked. Gems with multithreaded native extensions should provide an explicit thread initialization method, which the app only calls after the app server has forked. This requires cooperation in the entire gem ecosystem.

Does something like the dlopen workaround belong in Puma/Unicorn/iodine/Passenger? This is more of a philosophical question than a technical one. I cannot answer for Puma/Unicorn/iodine, but Passenger's philosophy is put user experience at the forefront, so we have chosen to include the dlopen workaround in the next Passenger version.

@ticky

This comment has been minimized.

Copy link
Author

commented Oct 11, 2017

Unless it has other side effects, I’d suggest implementing this within Puma, Unicorn and iodine would cause the fewest headaches for implementors and users.

Barring some incompatibility we haven’t yet encountered with initialising the Objective-C runtime within our Ruby process, I think it makes the most sense!

@boazsegev

This comment has been minimized.

Copy link

commented Oct 12, 2017

@mperham, I would generally agree that this seems like a Ruby concern rather than a specific gem (or a group of gems).

I would assume that Ruby (as a scripting language) should behave consistently across supposed environments without requiring code changes (except where different behaviors are desirable). For this reason, an OS specific workaround that's required for a consistent behavior across OS boundaries, should probably (IMHO) be part of the core Ruby code.

Having said that, I wonder if pushing this into a Ruby release is even an option.

@ticky, In general I find your approach very wise and balanced.

However, I doubt that it is safe to assume that the Ruby version of fork would be called whenever Ruby is running in the background.

Processes can be forked by C/Java extensions as well (using direct system calls). Besides, Ruby MRI is also available as an engine/library (much like V8 for Javascript) and could be executed from within an already running environment.

I wonder what price we pay for loading the Objective-C runtime, but it might be safer to preemptively load the Objective-C runtime during the Ruby VM initialization procedure (although it's somewhat high handed, as you pointed out).

@FooBarWidget

This comment has been minimized.

Copy link

commented Oct 12, 2017

I like the proposal of adding a modification to Ruby.

It is true that native libraries can call fork without going through the Ruby fork function. Having said that, I believe that such libraries already ensure that they perform async signal safe actions inside the child. And if they don't then I think that it should be up to them to fix that, or up to the wrapping gem to work around that, because those libraries would already cause problems even outside the context of Ruby.

It is also true that Ruby can be embedded in another environment. But Ruby forking in such environment is at present already a problem. I think it should be up to the embedder to add mechanisms to make Ruby's fork call safe, for example with pthread_atfork.

So I support the notion of having Ruby initialize the objc runtime upon calling #fork.

@ticky

This comment has been minimized.

Copy link
Author

commented Oct 12, 2017

Processes can be forked by C/Java extensions as well (using direct system calls).

That’s actually a good point, which I hadn’t considered!

So I support the notion of having Ruby initialize the objc runtime upon calling #fork.

I have just opened an issue on Ruby’s tracker for this. I hope we can work something nice out!

@FooBarWidget

This comment has been minimized.

Copy link

commented Oct 13, 2017

I've published a blog post on this subject. This blog post explains:

  • What the problem is and why it occurs
  • The current community concensus on what to do about this
  • What users can expect in the short term from various parties such as app servers authors
  • What they can do themselves

Hopefully this post brings more clarity to users.

@nateberkopec

This comment has been minimized.

Copy link
Member

commented Oct 13, 2017

Great post @FooBarWidget. Just to clarify some things from Puma's perspective:

  • I usually side with the user-experience side as well, and since Puma is the default for Rails I think it's extra important that we Just Work. I'm generally just watching and waiting this thread and the Ruby issue tracker to see what consensus emerges. If it looks like Ruby will quickly implement a fix, we may not do anything in Puma. But we've done some wild workarounds for Ruby bugs in the past too so I'm not against merging something to fix this here.
  • Since v2.1, Puma warns the user if it detects additional Ruby threads created during preloading.
@ticky

This comment has been minimized.

Copy link
Author

commented Oct 13, 2017

An update from the Ruby tracker, a patch for Ruby has been suggested and I’ve confirmed it mitigates this issue when it comes to Puma!

It works around it by linking Ruby itself to Foundation instead of CoreFoundation. CoreFoundation doesn’t initialise Objective-C, but as we know from this thread, Foundation does. 😄

@FooBarWidget

This comment has been minimized.

Copy link

commented Oct 13, 2017

@nateberkopec I think fixing it in Ruby only affects versions of Ruby from a specific version onward. What about users on older versions? Would it be reasonable to ask them to upgrade Ruby or would it be better if you activate the workaround on older Ruby versions?

Since v2.1, Puma warns the user if it detects additional Ruby threads created during preloading.

This is... an awesome idea! This will solve many of the mysterious issues people have with preforking.

@ticky

This comment has been minimized.

Copy link
Author

commented Oct 13, 2017

fixing it in Ruby only affects versions of Ruby from a specific version onward

That’s true, however, it seems like it’s both likely to be a thing worthy of back porting to any existing supported versions, and a very simple patch to apply if you need support for an even older version.

@boazsegev

This comment has been minimized.

Copy link

commented Oct 13, 2017

Would it be reasonable to ask them to upgrade Ruby or would it be better if you activate the workaround on older Ruby versions?

IMHO, since the issue effects High Sierra, I believe it is acceptable to expect users to upgrade to a High Sierra compatible Ruby version.

I also think that back porting the patch is wise.

@nateberkopec

This comment has been minimized.

Copy link
Member

commented Oct 13, 2017

Yeah, I agree with what @ticky and @boazsegev said. Also, since there is an environment variable setting which gives you "the old behavior", it's easier for us to just point users to that instead.

IMO "upgrade your ruby patchlevel" or "use this environment variable to go back to the way things were pre-2017" is an OK solution.

@boazsegev

This comment has been minimized.

Copy link

commented Oct 16, 2017

UPDATE: It seems a patch was merged with the Ruby trunk. This probably means the issue will be resolved in future Ruby releases (~~~the next 2.4.x version should be patched~~~ EDIT: nope, still waiting on a patch, maybe 2.5?).

@nateberkopec

This comment has been minimized.

Copy link
Member

commented Oct 16, 2017

Okay, because of the quick fix here I'm going to close this from our perspective. This will be fixed in Ruby 2.4.3 (which will probably be around Christmas), as of this writing backports to other Ruby minor versions are unknown.

To work around this issue today, I suggest just setting the OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES environment variable when working with Puma. This gets you the old, "unsafe" behavior. To understand why this is an imperfect solution, read @FooBarWidget's blog. Try not to set it permanently or in your .bash_profile or anything, it's a genuinely useful feature that you should probably be using - instead maybe add it to a Procfile or something like that.

@boazsegev

This comment has been minimized.

Copy link

commented Oct 19, 2017

As an update from the iodine server perspective:

The iodine server now includes a fix for this issue (starting at version 0.4.10), allowing people using iodine today to have a better experience.

The decision was made for two reasons:

  1. This allows High Sierra to be supported without requiring the Ruby runtime to be upgraded to a High Sierra compatible version.

  2. Waiting for Christmas (when I assume the next Ruby version will be released) seemed a bit of a wait...

I'm not sure everyone needs to do the same, but I thought it might be of interest.

@ticky

This comment has been minimized.

Copy link
Author

commented Oct 20, 2017

To work around this issue today, I suggest just setting the OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES environment variable when working with Puma.

I’d warn that as discussed above, this potentially masks other issues which this new behaviour is attempting to mask. Loading Foundation explicitly will avoid the “implicit Obj-C runtime initialisation in a fork” errors, but not mask similar but distinct errors in which your program actually triggers the behaviour this intends to disallow.

@nateberkopec

This comment has been minimized.

Copy link
Member

commented Oct 21, 2017

this potentially masks other issues which this new behaviour is attempting to mask

I think you meant "attempting to unmask" right?

My view is that Apple basically turned a feature into a bug here and made a helpful safety check cause a crash. Puma already warns when multiple threads exist before forking, so the safety check here is not as helpful as it might otherwise be. If people want the additional safety check Apple wants to provide here, they can upgrade to a newer Ruby version (when it comes out). Until then, they can live with the old behavior via the environment variable workaround.

@ticky

This comment has been minimized.

Copy link
Author

commented Oct 23, 2017

I think you meant "attempting to unmask" right?

Yep :)

Puma already warns when multiple threads exist before forking, so the safety check here is not as helpful as it might otherwise be.

The safety check won’t apply to any C extensions which may fork, though, right?

@nateberkopec

This comment has been minimized.

Copy link
Member

commented Oct 23, 2017

The safety check won’t apply to any C extensions which may fork, though, right?

Correct, we're only checking the Ruby Thread.list.

@boazsegev

This comment has been minimized.

Copy link

commented Oct 23, 2017

The safety check won’t apply to any C extensions which may fork, though, right?

@nateberkopec , I'm not sure what the common practice is, but I re-assesed the observation that language extensions (C / Java) could call fork directly.

It seems that Ruby has a "timer thread" running in the background. After reading through the Ruby source code I realized that calling fork directly might have negative effects on Ruby performance and scheduling.

Extensions that call fork directly might be considered buggy and could possibly be ignored as far as the safety check is concerned (on the grounds of these extensions leaving behind the Ruby scope).

As for the Thread.list, native threads don't work well with Ruby threads (due to the GIL and related data structures and signaling concerns). So the Thread.list could be considered complete as far as the safety check is concerned... with a single possible exception...

...however, some extensions might spawn new native threads that never call the Ruby API (iodine does this for IO flushing). These threads are obviously not in the Thread.list. But I think the safety warning can safely ignore these extensions, since they are clearly aware of these details and were designed with these considerations in mind.

@rhymes

This comment has been minimized.

Copy link

commented Dec 15, 2017

@nateberkopec @ticky unfortunately it seems the issue is still present even with Ruby 2.4.3

I just encountered it with Ruby 2.4.3, macOS High Sierra 10.13.2 and puma 3.11.0:

[10996] Puma starting in cluster mode...
[10996] * Version 3.11.0 (ruby 2.4.3-p205), codename: Love Song
[10996] * Min threads: 1, max threads: 1
[10996] * Environment: development
[10996] * Process workers: 2
[10996] * Preloading application
[10996] * Listening on tcp://0.0.0.0:5000
[10996] Use Ctrl-C to stop
[10996] - Worker 0 (pid: 11016) booted, phase: 0
[10996] - Worker 1 (pid: 11017) booted, phase: 0
I, [2017-12-15T19:27:32.407998 #11016] INFO -- : REQUEST: method=GET path=/api/v1/... user_agent=curl/7.57.0 time=2017-12-15T18:27:32Z ip=127.0.0.1 host=localhost params={}
objc[11016]: +[__NSPlaceholderDictionary initialize] may have been in progress in another thread when fork() was called.
objc[11016]: +[__NSPlaceholderDictionary initialize] may have been in progress in another thread when fork() was called. We cannot safely call it or ignore it in the fork() child process. Crashing instead. Set a breakpoint on objc_initializeAfterForkError to debug.
[10996] - Worker 0 (pid: 11060) booted, phase: 0

I've isolated it, in my code, to these three lines of code which crash the process with our without preloading:

Faraday.new(url: url) do |faraday|
  faraday.adapter :patron
end

@ticky's patch did the trick

@webark

This comment has been minimized.

Copy link

commented Dec 19, 2017

this still seems to be happening for me in puma's cluster mode, not in single mode.

$ > bundle exec puma -w 3
[49550] Puma starting in cluster mode...
[49550] * Version 3.10.0 (ruby 2.4.3-p205), codename: Russell's Teapot
[49550] * Min threads: 0, max threads: 16
[49550] * Environment: development
[49550] * Process workers: 3
[49550] * Phased restart available
[49550] * Listening on tcp://0.0.0.0:9292
[49550] Use Ctrl-C to stop
objc[49556]: +[__NSPlaceholderDictionary initialize] may have been in progress in another thread when fork() was called.
@rhymes

This comment has been minimized.

Copy link

commented Dec 19, 2017

@webark yeah the issue is because of the usage of forking in OSX and how it changed in High Sierra, try @ticky's patch here: #1421 (comment)

@ticky

This comment has been minimized.

Copy link
Author

commented Dec 20, 2017

@rhymes, @webark: the upstream Ruby patch hasn’t been backported to Ruby 2.4. It looks like it will in fact land in Ruby 2.5, which is forthcoming.

@nateberkopec, it might be worth reconsidering the “no Puma workaround” approach given there will potentially be people wanting to stay on 2.4 and I don’t have confirmation of whether any backport is planned.

@ticky

This comment has been minimized.

Copy link
Author

commented Dec 20, 2017

@rhymes

This comment has been minimized.

Copy link

commented Dec 20, 2017

@ticky amazing! thanks for the help and support!

@ticky

This comment has been minimized.

Copy link
Author

commented Dec 21, 2017

Update: a backport for 2.4.x has landed, it should be in the 2.4.4 release, though I suspect that won’t appear until the new year!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.