Skip to content

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

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
Vkt0r merged 2 commits into from
May 1, 2019
Merged

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

Vkt0r 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:

swifter/XCode/Sources/HttpServerIO.swift

Line 88 in 70ec292

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 •
edited
Loading

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 •
edited
Loading

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.

@Vkt0r Vkt0r force-pushed the threading-issue branch 2 times, most recently from 161488c to 8756aba Compare April 25, 2019 00:40
pvzig
pvzig approved these changes Apr 29, 2019
View reviewed changes
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 •
edited
Loading

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
Vkt0r added 2 commits April 30, 2019 23:56
@Vkt0r
Fix a crash when several request for the same URL are launched together
e994a25 
* 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
Update the CHANGELOG
cefe35f
@Vkt0r Vkt0r force-pushed the threading-issue branch from 5632b88 to cefe35f Compare May 1, 2019 03:56
@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
@Vkt0r
Merge pull request httpswift#399 from Vkt0r/threading-issue
a5dfa25 
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

Reviewers

@pvzig pvzig pvzig approved these changes

@damian-kolakowski damian-kolakowski Awaiting requested review from damian-kolakowski

@adamkaplan adamkaplan Awaiting requested review from adamkaplan

@ignatovSA ignatovSA Awaiting requested review from ignatovSA

@julien-c julien-c Awaiting requested review from julien-c

@marcc-orange marcc-orange Awaiting requested review from marcc-orange

@mbarnach mbarnach Awaiting requested review from mbarnach

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Vkt0r @swifter-bot @pvzig