core: Do not close event loop too early by Makman2 · Pull Request #4819 · coala/coala

Makman2 · 2017-10-31T22:53:22Z

... when scheduling all dependency bears. Bears that offload no tasks
may cause the event-loop to close too early while other bears had no
chance to submit their tasks.

Fixes #4505

shreyans800755 · 2017-11-01T18:09:54Z

-                    # process, as when no tasks were offloaded the event-loop
-                    # could hang up otherwise.
-                    self._cleanup_bear(bear)
+                    bears_without_tasks_to_cleanup.append(bear)


Can we keep this condition before looping over the tasks ?

Something like:

if not tasks: bears_without_tasks_to_cleanup.append(bear) continue for task in tasks ...

Might not save too many clock cycles, but it will make code cleaner.

Yeah why not, if you like I would also add an additional debug-message BearXYZ scheduled no tasks., so we still keep the information that a bear has been processed 👍

Hm just thinking if this should be done in a separate commit... This commit diff is smaller when not shifting the logic away^^

IMO this can be done in the same commit as we are adding this condition in this commit. It doesn't make sense to add something in first commit and change the same in subsequent one especially in same PR.

And there won't be a case where we only want to rollback one of the commits and not the other one. So, its better to keep single commit.

as we are adding this condition in this commit

which condition? The if is already there (if not tasks)

Oh my bad. Can be done in different commit.

shreyans800755 · 2017-11-04T11:53:01Z

-                    # process, as when no tasks were offloaded the event-loop
-                    # could hang up otherwise.
-                    self._cleanup_bear(bear)
+                    bears_without_tasks_to_cleanup.append(bear)


Oh my bad. Can be done in different commit.

shreyans800755 · 2017-11-04T11:56:24Z

        :param bears:
            A list of bear instances to be scheduled onto the process pool.
        """
+        bears_without_tasks_to_cleanup = []


bears_without_tasks_to_cleanup -> bears_without_tasks

shreyans800755 · 2017-11-04T11:57:32Z

+                    bears_without_tasks_to_cleanup.append(bear)
+
+        for bear in bears_without_tasks_to_cleanup:
+            # We need to recheck our runtime if something is left to process, as


Do we still need this comment ?

hm yeah... I think this is more or less self explaining 👍

shreyans800755 · 2017-11-04T11:58:04Z

        self.assertEqual(
            len(bear.dependency_results[BearA]), 1)

+    def test_run_many_dependency_bears_with_zero_tasks(self):


test_run_many_dependency_bears_with_zero_tasks -> test_run_multiple_dependency_bears_with_zero_tasks

shreyans800755 · 2017-11-04T11:59:02Z

        return (((i,), {}) for i in range(tasks_count))


+class DependentOnManyZeroTaskBearsTestBear(TestBearBase):


DependentOnManyZeroTaskBearsTestBear -> DependentOnMultipleZeroTaskBearsTestBear

Does it matter? :D

Just trying to be more accurate with words. 😃

shreyans800755 · 2017-11-05T17:06:23Z

ack aa720e2

shreyans800755 · 2017-11-05T20:43:19Z

ack a7b8e0b

Makman2 · 2017-11-05T21:06:52Z

@gitmate-bot rebase

... when scheduling all dependency bears. Bears that offload no tasks may cause the event-loop to close too early while other bears had no chance to submit their tasks. Fixes #4505

Moving the condition upwards makes code more understandable, as further scheduling code is not called at all because using now a `continue` statement. This change effectively doesn't change any behaviour, but makes control flow cleaner.

gitmate-bot · 2017-11-05T21:06:54Z

Hey! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost!

gitmate-bot · 2017-11-05T21:07:06Z

Automated rebase was successful!

Makman2 · 2017-11-06T13:42:15Z

@rultor merge

rultor · 2017-11-06T13:43:31Z

@rultor merge

@Makman2 OK, I'll try to merge now. You can check the progress of the merge here

rultor · 2017-11-06T13:46:01Z

@rultor merge

@Makman2 Done! FYI, the full log is here (took me 2min)

gitmate-bot added process/pending review size/XS area/core area/tests difficulty/medium importance/high type/bug labels Oct 31, 2017

shreyans800755 suggested changes Nov 1, 2017

View reviewed changes

shreyans800755 suggested changes Nov 4, 2017

View reviewed changes

co-robo added process/wip and removed process/pending review labels Nov 4, 2017

Makman2 force-pushed the Makman2/NEXTGEN/early-event-loop-close branch from 231b747 to 76f25b0 Compare November 5, 2017 15:55

gitmate-bot added process/pending review and removed process/wip labels Nov 5, 2017

Makman2 force-pushed the Makman2/NEXTGEN/early-event-loop-close branch from 76f25b0 to aa720e2 Compare November 5, 2017 15:56

gitmate-bot added process/approved The PR is approved and will be merged soon and removed process/pending review labels Nov 5, 2017

Makman2 added 2 commits November 5, 2017 21:06

core: Do not close event loop too early

7c865bf

... when scheduling all dependency bears. Bears that offload no tasks may cause the event-loop to close too early while other bears had no chance to submit their tasks. Fixes #4505

core: Move no-tasks-condition for scheduling up

5cbbe19

Moving the condition upwards makes code more understandable, as further scheduling code is not called at all because using now a `continue` statement. This change effectively doesn't change any behaviour, but makes control flow cleaner.

gitmate-bot force-pushed the Makman2/NEXTGEN/early-event-loop-close branch from aa720e2 to 5cbbe19 Compare November 5, 2017 21:07

gitmate-bot added process/pending review process/approved The PR is approved and will be merged soon and removed process/approved The PR is approved and will be merged soon process/pending review labels Nov 5, 2017

gitmate-bot removed the process/approved The PR is approved and will be merged soon label Nov 6, 2017

rultor merged commit 5cbbe19 into master Nov 6, 2017

Makman2 deleted the Makman2/NEXTGEN/early-event-loop-close branch November 6, 2017 13:46

gitmate-bot added the process/approved The PR is approved and will be merged soon label Nov 6, 2017

gitmate-bot removed the process/approved The PR is approved and will be merged soon label Jun 2, 2018

		return (((i,), {}) for i in range(tasks_count))


		class DependentOnManyZeroTaskBearsTestBear(TestBearBase):

Conversation

Makman2 commented Oct 31, 2017

Uh oh!

shreyans800755 Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Makman2 Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shreyans800755 Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shreyans800755 commented Nov 5, 2017

Uh oh!

shreyans800755 commented Nov 5, 2017

Uh oh!

Makman2 commented Nov 5, 2017

Uh oh!

gitmate-bot commented Nov 5, 2017

Uh oh!

gitmate-bot commented Nov 5, 2017

Uh oh!

Makman2 commented Nov 6, 2017

Uh oh!

rultor commented Nov 6, 2017

Uh oh!

rultor commented Nov 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

shreyans800755 Nov 1, 2017 •

edited

Loading

Makman2 Nov 1, 2017 •

edited

Loading

shreyans800755 Nov 2, 2017 •

edited

Loading