[Overlay/Queue]: Allow setting an optional value in setSpecific

[karwa]

Otherwise you can never clear anything you set

[MadCoder]

that is fairly reasonable but isn't this ABI breaking? @mwwa please review wrt this, I agree the patch makes sense and is useful.

[mwwa]

At this point I think this would need to be introduced as clearSpecific<T>(key:) so that we don't change the signature of setSpecific<T>(key:value:)

[karwa]

Urgh, it's disappointing that we're at that stage so early that we have to introduce new functions to patch flaws in existing ones.

Alternatively, we could add a DispatchSpecific subscript on DidpatchQueue which allows setting nil (IIRC we can have generic subscripts, but I may be wrong)

[karwa]

What I mean to say by that is: I wasn't aware binary compatibility was a concern for Swift 3. That's supposed to be the focus of Swift 4 stage 1, as I understand it.

[mwwa]

It's not a binary compatibility issue, it's a source compatibility problem if people have to change the signatures of their existing uses in order to be able to compile.

[karwa]

@mwwa Is it actually source-breaking? Swift automatically promotes types to their optionals, so any code which used the non-optional setter will continue to compile.

Also, this is technically a memory leak. I just encountered it in my app - thousands of DispatchSpecificKey allocations being left behind because something in Dispatch is retaining them (and can't be cleared). Each is 32 bytes so you lose 1K from just 32 of them. This is in a reasonably memory-constrained environment.

[karwa]

@MadCoder can we make any progress on this? As I mentioned it shouldn't be source-breaking (although the symbol would change, so it is ABI-breaking)

[mwwa]

Good point with the source compat. That looks ok then. This needs to be cloned for the mainline Dispatch overlay too (in the apple/swift repo).

@swift-ci please test.

[karwa]

Failure is unrelated. It's the Foundation NSURLSession one that crops up from time to time.

'
11:04:00.845
Test Case 'TestURLSession.test_dataTaskWithURL' started at 11:04:00.845
fatal error: Transfer completed, but there's no currect request.: file Foundation/NSURLSession/NSURLSessionTask.swift, line 794
/home/buildnode/jenkins/workspace/swift-corelibs-libdispatch-PR-Linux/swift/utils/build-script-impl: line 249: 9670 Illegal instruction (core dumped) "$@"
/home/buildnode/jenkins/workspace/swift-corelibs-libdispatch-PR-Linux/swift/utils/build-script: fatal error: command terminated with a non-zero exit status 132, aborting
Build step 'Execute shell' marked build as failure
'

[karwa]

I think the Foundation test has been fixed now. Can we run it again?

[parkera]

@swift-ci please test

[karwa]

huh, ¯_(ツ)_/¯

Foundation contains a grand total of one (1) DispatchSpecificKey, on NSOperation. It's never referenced anywhere else, and NSOperation is never referenced from any other types in Foundation. So I'm pretty sure this change didn't break anything.

[karwa]

I don't know why the test is failing (some assertion failure in kqueue), but I'm sure this patch has nothing to do with it.

[karwa]

Can we test this again? I'd like to try get it in for the 3.1 branch date

[karwa]

Ping? This has already been merged in the Darwin SDK overlay.

[das]

@swift-ci please test

[das]

tests pass, so lets cleanup this discrepancy for 4.0