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

Remove deprecated BlockingTrioPortal #1574

Merged
merged 5 commits into from
Jun 9, 2020

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Jun 4, 2020

TODO

  • coverage
  • newsfragment

@@ -227,17 +213,6 @@ def _run_fn_as_system_task(cb, fn, *args, trio_token=None):
"this thread wasn't created by Trio, pass kwarg trio_token=..."
)

# TODO: This is only necessary for compatibility with BlockingTrioPortal.
Copy link
Member

Choose a reason for hiding this comment

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

I think we might still need this for the case where the user passes the trio_token?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea, I just grepped for BlockingTrioPortal, but I'm not familiar with those APIs. @njsmith Can you please confirm?

Copy link
Member

Choose a reason for hiding this comment

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

I think @oremanj is right. The test case would be something like:

async def test():
    trio_token = trio.lowlevel.current_trio_token()
    with pytest.raises(RuntimeError):
        trio.from_thread.run_sync(lambda: None, trio_token=trio_token)

If we fail to detect we're calling from_thread inside the Trio thread, then this will deadlock.

@codecov
Copy link

codecov bot commented Jun 4, 2020

Codecov Report

Merging #1574 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1574      +/-   ##
==========================================
- Coverage   99.69%   99.69%   -0.01%     
==========================================
  Files         111      111              
  Lines       13869    13840      -29     
  Branches     1060     1055       -5     
==========================================
- Hits        13827    13798      -29     
  Misses         27       27              
  Partials       15       15              
Impacted Files Coverage Δ
trio/__init__.py 100.00% <ø> (ø)
trio/_threads.py 100.00% <ø> (ø)
trio/tests/test_exports.py 97.22% <ø> (-0.15%) ⬇️
trio/tests/test_threads.py 100.00% <100.00%> (ø)

@pquentin
Copy link
Member Author

pquentin commented Jun 4, 2020

The coverage change happens in public_modules:

Screenshot 2020-06-04 at 10 55 11

I think we still want to ignore names starting with _ for future deprecations, so should I add two nocover pragmas?

@njsmith
Copy link
Member

njsmith commented Jun 4, 2020

I think we still want to ignore names starting with _ for future deprecations, so should I add two nocover pragmas?

That seems reasonable, yeah.

@njsmith
Copy link
Member

njsmith commented Jun 9, 2020

Looks like there are a few more coverage changes still?

@pquentin pquentin force-pushed the remove-blocking-trio-portal branch from fb05527 to ddf2483 Compare June 9, 2020 07:29
@njsmith
Copy link
Member

njsmith commented Jun 9, 2020

So close... now the last line of the new test is causing a partial coverage line, because the lambda: None never gets called. Good place for a # pragma, or I guess we could make it more specific by writing it like

def not_called():  # pragma: no cover
    assert False

from_thread_run_sync(not_called, trio_token=trio_token)

Otherwise, coverage complains that we did not run the lambda.
@pquentin pquentin force-pushed the remove-blocking-trio-portal branch from f65703e to f5a2191 Compare June 9, 2020 08:15
@pquentin
Copy link
Member Author

pquentin commented Jun 9, 2020

Thanks for investigating! This was subtle. I found about it independently, and only because I generated an HTML report that explcitly mentioned the lambda. I think passing a None functiom is simpler, and it fixes the issue too.

@njsmith
Copy link
Member

njsmith commented Jun 9, 2020

We'll just have to switch back to passing a function when we eventually get type hints working :-) But, up to you. Feel free to merge when you're happy.

@pquentin pquentin merged commit 9474dff into python-trio:master Jun 9, 2020
@pquentin pquentin deleted the remove-blocking-trio-portal branch June 9, 2020 09:37
@pquentin
Copy link
Member Author

pquentin commented Jun 9, 2020

Thanks! Right, I forgot about mypy, your version is better, thanks.

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.

3 participants