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

Jump to bottom

[core]: Improves performance of coroutines/tasks and fixes event.stop() #213

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[core]: Improves performance of coroutines/tasks and fixes event.stop() #213

wants to merge 3 commits into from

Conversation

prologic
Copy link
Member

Closes #136 and #86

apollo13
apollo13 reviewed Jan 23, 2017
View reviewed changes
Copy link
Contributor

@apollo13 apollo13 left a comment •
edited

Choose a reason for hiding this comment

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

See my comment. If that change is not possible I'll approve it, otherwise I'd like to see that change since it would make the whole testcase smaller.

Also we should check the test failures :D

tests/core/test_call_stop.py Outdated
return app


def test_call_stop(manager, watcher, app):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to get this fancy here (ie with manager in a new thread etc).
Wouldn't this work too:

def test_call_stop():
  app = App()
  x = app.fire(test())
  app.tick()
  
  assert x.success # Or whatever is needed to check that the event ran properly.
  assert.x.value is None

This way we do not run into the overhead of creating new threads and bussy waiting for events which makes tests slower :D

@prologic
Copy link
Member Author

This breaks all of the assumptions around streaming and handling generated responses via yield and returning Iterator respones in circuits.web :/

@prologic
Copy link
Member Author

@spaceone I'd be interested to see if your circuits.http breaks on top of this PR? Can you test it out?

@spaceone
Copy link
Contributor

@prologic yes, I'll test it :) but give me some days ^^

@spaceone
Copy link
Contributor

@prologic I love this change! It works brilliantly with circuits.http!

@prologic
Copy link
Member Author

prologic commented Jan 29, 2017 via email

@prologic prologic force-pushed the process_tasks_faster branch 2 times, most recently from 8d507e3 to b0b2eda Compare January 31, 2017 01:14
@codecov-io
Copy link

codecov-io commented Jan 31, 2017 •
edited

Codecov Report

❗ No coverage uploaded for pull request base (master@beec20b).

@@            Coverage Diff            @@
##             master     #213   +/-   ##
=========================================
  Coverage          ?   77.06%           
=========================================
  Files             ?       80           
  Lines             ?     6662           
  Branches          ?        0           
=========================================
  Hits              ?     5134           
  Misses            ?     1528           
  Partials          ?        0
Impacted Files Coverage Δ
circuits/core/manager.py 93.78% <100%> (ø)
circuits/web/http.py 87.11% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update beec20b...c675acb. Read the comment docs.

spaceone
spaceone reviewed Jan 31, 2017
View reviewed changes
Copy link
Contributor

@spaceone spaceone left a comment

Choose a reason for hiding this comment

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

Will this change the API of on_complete handlers? The websocket components uses it.

@prologic prologic closed this Oct 25, 2020
@prologic prologic deleted the process_tasks_faster branch October 25, 2020 08:52
@spaceone
Copy link
Contributor

@prologic Why do you close this and remove the branch?

@prologic
Copy link
Member Author

Ahh I was doing some Github cleanup! Sorry, do you want to take over the branch/PR?

@spaceone
Copy link
Contributor

I can't promise to work on this currently, but I still would need it. I restore the branch to not remove the current working state.

@spaceone spaceone restored the process_tasks_faster branch October 26, 2020 08:43
@spaceone spaceone reopened this Oct 25, 2021
@spaceone spaceone mentioned this pull request Oct 25, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apollo13 apollo13 apollo13 left review comments

@spaceone spaceone spaceone left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

event.stop() not working for yielding handlers
4 participants
@prologic @spaceone @codecov-io @apollo13