Skip to content

TaskDescription.waitForAllTaskTermination() on waitOnCommand()#519

Merged
robrix merged 2 commits intoCarthage:masterfrom
norio-nomura:wait-for-all-task-termination
Jun 5, 2015
Merged

TaskDescription.waitForAllTaskTermination() on waitOnCommand()#519
robrix merged 2 commits intoCarthage:masterfrom
norio-nomura:wait-for-all-task-termination

Conversation

@norio-nomura
Copy link
Copy Markdown
Contributor

This is rebased version of #496.

Fix segmentation fault on termination of Carthage.

Segmentation fault on termination of Carthage 0.7.4 beta-2 happens following scenario:

  1. launchTask's thread call sendCompleted() and kick main thread
    2a. main thread wake up and call exit()
    2b. launchTask's thread continue disposing
  2. main thread cause segmentation fault.

Waiting completed from launchTask's thread is not enough for fixing the segmentation fault.
It needs waiting actual termination of launchTask's thread

This PR depends on Carthage/ReactiveTask#24

#474 (comment)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should this be finer-grained, e.g. waiting per-task when they’re executed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can't determine where I should place dispatch_group_wait() on another place.
RAC's completed event timing is not fit for place it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The segmentation fault happens while disposing after completed event from launch task's thread and main thread calling exit() concurrently.
Waiting per-task will need waiting some RAC event. That will be same what just current implementation is doing.
I think fixing the segmentation fault needs doing outside of RAC event, and waiting per-task without RAC event may be too complicated unnecessarily.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Okay, thanks for looking into this!

@robrix
Copy link
Copy Markdown

robrix commented Jun 5, 2015

I’ve merged the dependency and have a beta tag of ReactiveTask to include it.

Could you please update the Cartfile & submodules to pin to that tag?

@robrix
Copy link
Copy Markdown

robrix commented Jun 5, 2015

Once that’s done I think we can merge and release a new beta of Carthage. I may not be aesthetically content with waiting for all child processes like this, but I can’t argue with its results.

@norio-nomura
Copy link
Copy Markdown
Contributor Author

I've pushed the commit updating the Cartfile & submodules to pin to 0.7-beta1

@robrix
Copy link
Copy Markdown

robrix commented Jun 5, 2015

Thanks very much!

robrix pushed a commit that referenced this pull request Jun 5, 2015
TaskDescription.waitForAllTaskTermination() on waitOnCommand()
@robrix robrix merged commit c5500a4 into Carthage:master Jun 5, 2015
@norio-nomura
Copy link
Copy Markdown
Contributor Author

You're welcome. 🙇

@norio-nomura norio-nomura deleted the wait-for-all-task-termination branch June 6, 2015 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants