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

publish should be called when the state is changed #1759

Merged
merged 2 commits into from Dec 9, 2018

Conversation

@ian-otto
Copy link
Contributor

@ian-otto ian-otto commented Nov 21, 2018

publish should not be called during every wait cycle, only after the wait has found the correct state

What kind of change does this PR introduce?

  • bug fix
  • feature
  • docs update
  • tests/coverage improvement
  • refactoring
  • other

What is the related issue number (starting with #)

#1758

What is the current behavior? (You can also link to an open issue here)

#1758

What is the new behavior (if this is a feature change)?

N/A

Other information:

Checklist:

  • I think the code is well written
  • I wrote good commit messages
  • I have squashed related commits together after the changes have been approved
  • Unit tests for the changes exist
  • Integration tests for the changes exist (if applicable)
  • I used the same coding conventions as the rest of the project
  • The new code doesn't generate linter offenses
  • Documentation reflects the changes
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences
publish should not be called during every wait cycle, only after the wait has found the correct state
Copy link
Member

@webknjaz webknjaz left a comment

Currently, I'm not sure about the impact of this change. Let's see what @jaraco thinks.

@webknjaz webknjaz requested a review from jaraco Nov 24, 2018
@@ -375,7 +375,7 @@ def wait(self, state, interval=0.1, channel=None):

while self.state not in states:
time.sleep(interval)
self.publish(channel)
self.publish(channel)

This comment has been minimized.

@webknjaz

webknjaz Nov 24, 2018
Member

Remark: I've checked magicbus and it also has publish inside of the loop.
https://github.com/cherrypy/magicbus/blob/master/magicbus/base.py#L335

cc @aminusfu

This comment has been minimized.

@ian-otto

ian-otto Nov 25, 2018
Author Contributor

Is it possible this was just an error that was propagated to other libraries? Is there a commit somewhere where that was specifically added?

This comment has been minimized.

@webknjaz

webknjaz Nov 25, 2018
Member

Yea.. It's a huge wholesale decade old commit: f5d026c#diff-be33a4f55d59dfc70fc6452482f3a7a4R291. Sure it got into magicbus after cherrypy.

But it's hard to guess what was the reason for putting it like this. That's why I'm tagging guys who might know better.

cc @jaraco

@jaraco
Copy link
Member

@jaraco jaraco commented Dec 9, 2018

Sorry to let this linger so long. I've reviewed this superficially twice but I continue to feel like there's no clear answer. I agree the code itself seems wrong. What I'm struggling with is determining what this code is intended to do and if it still accomplishes the intention in this new form. I'm trying to devise a test or some other action to improve my confidence in the change.

@jaraco
Copy link
Member

@jaraco jaraco commented Dec 9, 2018

I removed the publish call altogether:

diff --git a/cherrypy/process/wspbus.py b/cherrypy/process/wspbus.py
index 8f762ef0..4af4c752 100644
--- a/cherrypy/process/wspbus.py
+++ b/cherrypy/process/wspbus.py
@@ -375,7 +375,6 @@ class Bus(object):

         while self.state not in states:
             time.sleep(interval)
-            self.publish(channel)

     def _do_execv(self):
         """Re-execute the current process.

Then ran the CherryPy tests, and they all passed. That suggests to me that this code isn't covered by the tests and isn't essential to CherryPy in its typical use.

Given our inability to test this functionality, I'm inclined to rely on the evidence described in the ticket for why this change produces a preferred behavior and accept it.

@jaraco jaraco merged commit 4515443 into cherrypy:master Dec 9, 2018
1 of 4 checks passed
1 of 4 checks passed
LGTM analysis: Python Fetching git commits
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@wip
WIP Ready for review
Details
Copy link
Member

@jaraco jaraco left a comment

I reviewed the code in a simple comment. Leaving this comment to close the requested review.

@jaraco
Copy link
Member

@jaraco jaraco commented Dec 9, 2018

This change is being released as v18.1.0.

@webknjaz
Copy link
Member

@webknjaz webknjaz commented Dec 9, 2018

Thanks, I also was confused by all of this and wasn't confident about accepting it myself. The decision to remove it seems fair.

@webknjaz webknjaz mentioned this pull request Dec 24, 2018
1 of 3 tasks complete
@jaraco
Copy link
Member

@jaraco jaraco commented Mar 26, 2019

We need to revert this change. As discovered in cherrypy/cheroot#99 and declared by the docs, the 'main' event is supposed to fire periodically. After this change, the 'main' event no longer fires periodically.

@jaraco jaraco mentioned this pull request Mar 26, 2019
10 of 15 tasks complete
@webknjaz
Copy link
Member

@webknjaz webknjaz commented Apr 8, 2019

Right, even integration with other things like http://docs.cherrypy.org/en/latest/deploy.html#tornado requires that.

@jaraco can we just have if channel == 'main': self.publish() there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants