[triplef]

libdispatch only wraps the work it runs on its worker threads in autorelease pools if _dispatch_begin_NSAutoReleasePool and _dispatch_end_NSAutoReleasePool are set. Nothing sets them, so objects autoreleased by dispatched blocks are only released when the worker thread exits (libobjc2 empties the thread's pool in its TLS destructor). For long-lived worker threads, that means they effectively leak.

This change has +[NSAutoreleasePool initialize] point the hooks at objc_autoreleasePoolPush / objc_autoreleasePoolPop when building with libdispatch and the ARC-compatible runtime. These hooks are SPI meant for Foundation (dispatch/private.h marks them "SPI for CoreFoundation/Foundation ONLY"), and base links against libdispatch directly, so on Windows the variables are reached through the DLL import as usual. Any hooks that were already set are left alone, and the end hook is set before the begin hook so a worker thread can't push a pool it then fails to pop.

Why not libobjc2?

libobjc2 tries to install these hooks in init_runtime() (loader.c), but that code has never run:

• It declares weak definitions of the hook variables and then tests their values (always NULL), not their addresses, so the assignments are never made.
• Even if fixed, libobjc2 doesn't link against libdispatch (libdispatch depends on libobjc2 for the blocks runtime, not the other way round). On ELF it would depend on symbol interposition, and on Windows the libobjc2 DLL's definitions are separate from the variables in the libdispatch DLL, so it can't reach them at all.

I’ve opened gnustep/libobjc2#436 to remove that code from libobjc2.

[rfm]

This looks reasonable so far, but incomplete.

I think the ARC_RUNTIME guard is wrong (since the availability/use of libdispatch is independent of ARC).

I think this needs to use the correct pool creation/destruction for the way the library is configured, so if objc_autoreleasePoolPop and objc_autoreleasePoolPush don't exist it would need to call small functions to create and store a pool in one case and to destroy that pool in the other.

Those functions could be called objc_autoreleasePoolPush() and objc_autoreleasePoolPop() and implemented in Source/ObjectiveC2 so they are built as part of the library iff the runtime doesn't implement them.

[triplef]

Yeah good points, sorry for not spotting that. Does the new version look better?

[rfm]

Yes, that looks good now.
The one thing missing is a testcase to demonstrate that it actually works.
However, from visual inspection I have a pretty high degree of confidence, so I think it could be merged even if you don't want to write a testcase.
Let me know what you want to do.

[triplef]

Good point, I’ve added tests. (The Android tests seem to be flakey?)

[rfm]

The Android tests seem to be flakey?
Yes. I have noticed that there seem to be a variety of intermittent test failures on different systems that, because they don't occur on my systems, I have been unable to track down and fix.
Android has an intermittent NSOperation testcase failure that I've seen on no other system.
Various other systems have intermittent failures in CI:
NSStream socket.m (I suspect this is because the test connects to www.w3.org and the failure is related to temporary loss of connectivity, but I'd like to fix the crash as well as changin the testcase to not depend on an external site.
NSTask test crashes occasionally, but I've never seen it on my dev systems.
NSURLSession: this class has never worked reliably :-( It used to hang in libdispatch quite often, but even now that it's not using that, there is clearly some issue (Hugo is looking into fixing some possible race condition, so I have hope here).

[rfm]

I tried the new testcase: it didn't work (the setup I created for testing this is a system using libdispatch with gcc), because it's conditional on blocks rather than libdispatch.

What we need is a testcase for libdispatch wrapping a dispatched function (rather than a block) with autorelease.
I suppose a second test for block support would be nice if we think that libdispatch will behave differently (with regard to autorelease hooks) when executing a block than it does when executing a function.

[triplef]

Thanks. The test case should now also run when no blocks are available, and test both with and without blocks.

[rfm]

Yes, that looks good.

But still, one more thought. I realised that dispatch/private.h is not normally installed.
It's kind of stupid if a hook we need to use is only declared in a private header, but perhaps it would be worth looking for the symbol in the library at configure time rather than gating on the header?

[triplef]

Good point, should be done now.

[rfm]

Works for me. Thanks.