fix HTTPCookieStorage threading issues #1182

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.

Sign up for GitHub

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

Jump to bottom

Merged

swift-ci merged 1 commit into swiftlang:master from weissi:jw-fix-httpcookiestorage-threading

Sep 21, 2017

Contributor

weissi commented Aug 21, 2017

HTTPCookieStorage isn't thread-safe at all but should be.


          fix HTTPCookieStorage threading issues

19f61e9

weissi requested a review from parkera

August 21, 2017 16:51

Contributor Author

weissi commented Aug 21, 2017

@swift-ci please test

Contributor

ianpartridge commented Aug 22, 2017

weissi requested a review from pushkarnk

August 23, 2017 10:52

Contributor Author

weissi commented Aug 23, 2017

@parkera / @pushkarnk sorry for prodding, mind letting me know what you think here? I want to some more concurrency fixes and the more open PRs I have in this area the harder it gets as they'll start colliding.

phausler reviewed

View reviewed changes

Foundation/HTTPCookieStorage.swift

@@ @@ -103,10 +121,12 @@ open class HTTPCookieStorage: NSObject { @@
                   */
                   open class var shared: HTTPCookieStorage {
                       get {
-                          if sharedStorage == nil {
-                              sharedStorage = HTTPCookieStorage(cookieStorageName: "shared")
+                          return sharedSyncQ.sync {

Contributor

phausler Aug 31, 2017

this looks like a dispatch_once style; shouldn't that be fine as a lazy var?

Contributor Author

weissi Sep 11, 2017

lazy vars aren't thread safe (https://bugs.swift.org/browse/SR-1042)
there's no dispatch_once in Swift

Pratik-Somaiya Sep 11, 2017

@weissi - How about open static var shared: HTTPCookieStorage?

Foundation/HTTPCookieStorage.swift

+                          if #available(OSX 10.12, iOS 10.0, tvOS 10.0, watchOS 3.0, *) {
+                              dispatchPrecondition(condition: DispatchPredicate.onQueue(self.syncQ))
+                          }
+                          return self._allCookies

Contributor

phausler Aug 31, 2017

self. is not needed here

Contributor Author

weissi Sep 11, 2017

what's wrong with the self.? I think that's actually a very important piece of information. Otherwise it just looks like a local variable. The self. makes it very implicit that this is a class property and I believe that makes it more obvious to others that there are possible concurrency concerns.

Foundation/HTTPCookieStorage.swift

+                          if #available(OSX 10.12, iOS 10.0, tvOS 10.0, watchOS 3.0, *) {
+                              dispatchPrecondition(condition: DispatchPredicate.onQueue(self.syncQ))
+                          }
+                          self._allCookies = newValue

Contributor

phausler Aug 31, 2017

again self. is redundant

Contributor Author

weissi Sep 11, 2017

see above

Foundation/HTTPCookieStorage.swift

-                      for key in cookies0.keys {
-                          if let cookie = createCookie(cookies0[key]!) {
-                              allCookies[key] = cookie
+                      self.syncQ.sync {

Contributor

phausler Aug 31, 2017

again self. is redundant

Foundation/HTTPCookieStorage.swift

-                              theCookies = Array(self.allCookies.values)
-                      }
-                      return theCookies
+                      return Array(self.syncQ.sync { self.allCookies.values })

Contributor

phausler Aug 31, 2017

ditto on self.

Foundation/HTTPCookieStorage.swift

@@ @@ -138,7 +160,7 @@ open class HTTPCookieStorage: NSObject { @@
                       same name, domain and path, if any.
                   */
                   open func setCookie(_ cookie: HTTPCookie) {
-                      workQueue.sync {
+                      self.syncQ.sync {

Contributor

phausler Aug 31, 2017

ditto on self.

Foundation/HTTPCookieStorage.swift

+                  private var _allCookies: [String: HTTPCookie]
+                  private var allCookies: [String: HTTPCookie] {
+                      get {
+                          if #available(OSX 10.12, iOS 10.0, tvOS 10.0, watchOS 3.0, *) {

Contributor

phausler Aug 31, 2017

what about linux?

Contributor Author

weissi Sep 11, 2017 •

edited

Loading

@phausler Linux seems to be handled in the *:

$ cat test.swift
if #available(OSX 10.12, iOS 10.0, tvOS 10.0, watchOS 3.0, *) {
    print("yes")
} else {
    print("no")
}
$ swift test.swift
yes
$ uname -s
Linux

Pratik-Somaiya Sep 11, 2017

I agree to @weissi

Pratik-Somaiya reviewed

View reviewed changes

Foundation/HTTPCookieStorage.swift

+                      if #available(OSX 10.12, iOS 10.0, tvOS 10.0, watchOS 3.0, *) {
+                          dispatchPrecondition(condition: DispatchPredicate.onQueue(self.syncQ))
+                      }

Pratik-Somaiya Sep 2, 2017

Are we not supporting linux?

Contributor Author

weissi Sep 11, 2017

the * seems to match Linux

Pratik-Somaiya Sep 11, 2017

@weissi - Yup agreed.

pushkarnk approved these changes

View reviewed changes

Member

pushkarnk left a comment

LGTM

Member

pushkarnk commented Sep 21, 2017

@swift-ci please test and merge

Member

pushkarnk commented Sep 21, 2017

******************** TEST 'Swift(linux-x86_64) :: ClangImporter/clang_builtins.swift' FAILED ********************

Contributor Author

weissi commented Sep 21, 2017

clang: �[0;1;31merror: �[0mno such file or directory: 'tools/SourceKit/tools/sourcekitd-test/CMakeFiles/sourcekitd-test.dir/sourcekitd-test.cpp.o'�[0m
clang: �[0;1;31merror: �[0mno such file or directory: 'tools/SourceKit/tools/sourcekitd-test/CMakeFiles/sourcekitd-test.dir/TestOptions.cpp.o'�[0m
clang: �[0;1;31merror: �[0mno such file or directory: 'lib/libsourcekitdInProc.so'�[0m
clang: �[0;1;31merror: �[0mno such file or directory: 'lib/swift/linux/libswiftCore.so'�[0m
clang: �[0;1;31merror: �[0mno such file or directory: 'lib/libSourceKitSupport.a'�[0m
clang: �[0;1;31merror: �[0mno such file or directory: 'lib/libswiftBasic.a'�[0m
clang: �[0;1;31merror: �[0mno such file or directory: 'lib/libswiftDemangling.a'�[0m
clang: �[0;1;31merror: �[0mno such file or directory: 'lib/libswiftSyntax.a'�[0m

there seem to be lots of files missing for some reason. Suspecting something went wrong with Jenkins' workspace. Retrying the tests.

Contributor Author

weissi commented Sep 21, 2017

@swift-ci please test and merge

swift-ci merged commit 6a1f875 into swiftlang:master

weissi deleted the jw-fix-httpcookiestorage-threading branch

September 21, 2017 10:52

ianpartridge mentioned this pull request

[4.0] Fix HTTPCookieStorage threading issues #1251

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet