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

Refactor cleanup_ctx implementation #3350

Closed
asvetlov opened this issue Oct 18, 2018 · 7 comments · Fixed by #3352
Closed

Refactor cleanup_ctx implementation #3350

asvetlov opened this issue Oct 18, 2018 · 7 comments · Fixed by #3352

Comments

@asvetlov
Copy link
Member

When I was working on type-annotating web.Application I found the inconsitency in cleanup_ctx implementation introduced by #3194

  1. cleanup_ctx.append() acccepts a generator instance but iteration over cleanup_ctx content returns internal objects.
  2. Removing an item from cleanup_ctx doesn't unregister on_startup item.
  3. cleanup_ctx.insert(0, gen) doesn't register a new on_startup item.

I afraid that mixing on_startup and cleanup_ctx is impossible to implement in a non-controversial way.
Maybe we need to revert the change back.

/cc @Maillol

@asvetlov asvetlov added this to the 3.5 milestone Oct 18, 2018
@aio-libs-bot
Copy link

GitMate.io thinks possibly related issues are #985 (Refactor ProxyConnector), #402 (Refactor reify), #20 (Refactor files handling.), #979 (DRAFT, New TestClientApp implementation), and #314 (Flow control implementation problems).

@Maillol
Copy link
Contributor

Maillol commented Oct 19, 2018

CleanupContext can have a FrozenList to store cleanup_ctx_item._exit instead of use itself.
CleanupContext store in itself the original generator accepted during the append.

We can add both dict inside CleanupContext

  • generator ==> cleanup_ctx_item._exit
  • generator ==> cleanup_ctx_item._enter

To remove on_startup item during the CleanupContext.__delitem__.

I can proposal a new implementation this weekend.

@asvetlov
Copy link
Member Author

Remove can be done by __setitem__ too: app.cleanup_ctx[1:3] = [a, b, c, d].
Please note: the number for removed items is not equal to the number of inserted.
Also app.cleanup_ctx.insert(1, a) exists.

@Maillol
Copy link
Contributor

Maillol commented Oct 20, 2018

I can easily fix the issue with __delitem__ / remove and type returned by the iteration.

But with insert I don't know which behavior to choose:

app.cleanup_ctx.append(c1)
app.on_startup.append(s1)
app.cleanup_ctx..append(c2)
app.cleanup_ctx.insert(1, c3) 
# When c3 should be executed ? before or after s1 ? 

I don't see how we can keep order with insert __setitem__ and slice.`

@asvetlov
Copy link
Member Author

asvetlov commented Oct 20, 2018

I don't see how we can keep order with insert __setitem__ and slice

That's why I propose to revert the change.

Now aiohttp opens too much implementation details.
There is a possibility to expose a new limited api for signals, deprecate existing one and eventually remove it; but really the process takes more than a year.

@Maillol
Copy link
Contributor

Maillol commented Oct 21, 2018

Yes I thinks is better to revert the change.

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants