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

Fix a crash when several request for the same URL or route are launched together #399

Merged
merged 2 commits into from
May 1, 2019

Conversation

Vkt0r
Copy link
Member

@Vkt0r Vkt0r commented Apr 23, 2019

This issue was detected by one of my colleagues during some UI Tests research using Xcode 10.2. When I started to test the branch with the issue using Charles I detected an issue coming from several requests being launched in very close intervals when they belong to similar routers or even the same route.

The issue was causing an EXC_BAD_INSTRUCTION:

Screen Shot 2019-04-23 at 5 32 39 PM

Once I enabled the Thread Sanitizer for the test suite I was able to follow the path to localize the issue causing the thread race condition and it was coming from:

strongSelf.handleConnection(socket)

Apparently, there were some issues during starting and stopping new connections during concurrent requests. I was able to reproduce it in the library simulating several requests concurrent and serial with time differences in the new ServerThreadingTests.

This PR can be resumed in the following steps:

  • Fix a crash when several requests for the same URL are launched concurrently causing a race condition
  • Fix an issue in the testStopActiveConnection causing the URLSession.share singleton was stuck in the thread and it wasn't working in subsequent tests
  • Refactor the SwifterTestsHttpRouter to reuse the HttpRouter object
  • Add new tests for the threading issue
  • Include the new tests added to the XCTManifests.swift

@Vkt0r Vkt0r added the bug label Apr 23, 2019
@swifter-bot
Copy link

swifter-bot commented Apr 23, 2019

1 Message
📖 Hey, @Vkt0r 👋.

Generated by 🚫 Danger

@Vkt0r Vkt0r force-pushed the threading-issue branch 7 times, most recently from ea33963 to 161488c Compare April 24, 2019 21:55
@Vkt0r
Copy link
Member Author

Vkt0r commented Apr 24, 2019

There is something weird with one of the new tests in CircleCI specifically (testShouldHandleTheSameRequestWithDifferentTimeIntervals ) for the Swift Package Manager, it's failing all the time for the second request and locally is working every time for me.

I noticed CircleCI is using the Target x86_64-apple-darwin18.2.0 and in my current machine I've x86_64-apple-darwin18.5.0, so I think there is something that was changed. I'm disabling these for the moment and opening it a ticket to keep track of the issue. Once CircleCI update it's target version we can go back and try it again. They mentioned it will be soon once they are ready to support Xcode 10.2.1.

In Linux as they are not working at all also. I run it locally with the same Swift 5.0 docker image and both are stuck running. So a ticket will be open to tracking the issue.

Copy link
Collaborator

@pvzig pvzig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Vkt0r
Copy link
Member Author

Vkt0r commented Apr 29, 2019

Thanks, @pvzig I want to run some tests again before merging this PR as I found something interesting with the change I didn't saw before.

@Vkt0r Vkt0r force-pushed the threading-issue branch 2 times, most recently from faf3a6e to 90a99cd Compare May 1, 2019 03:45
* Fix a crash when several request for the same URL are launched concurrently regarding a race condition
* Fix an issue in the `testStopActiveConnection` causing the `URLSession.share` singleton was stuck in the thread and it doesn’t work in another tests
* Refactor the `SwifterTestsHttpRouter` to reuse the `HttpRouter` object
* Add new tests for the threading issue
* Include the new tests added to the `XCTManifests.swift`
* Update the XCTManifests.swift path in the swiftlint config file
* Rename the jobs in CircleCI
* Update the swift-tools-version for the `Package.swift`
@Vkt0r Vkt0r merged commit 88996c0 into httpswift:stable May 1, 2019
@Vkt0r Vkt0r deleted the threading-issue branch May 1, 2019 04:02
tomieq pushed a commit to tomieq/swifterfork that referenced this pull request Apr 1, 2021
Fix a crash when several request for the same URL or route are launched together
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants