Skip to content

Conversation

@grzgrzgrz3
Copy link
Contributor

@grzgrzgrz3grzgrzgrz3 commented Jun 18, 2017

Asyncio on shutdown do not send shutdown confirmation to the other side.
_SSLPipe after doing unwrap is calling shutdown calback where transport
is closed and quit ssldata wont be sent. Having this callback in
_SSLPipe feels wrong and its removed.

Removing callback from _SSLPipe.shutdown should not break any api.

https://bugs.python.org/issue30698

@mention-bot
Copy link

@grzgrzgrz3, thanks for your PR! By analyzing the history of the files in this pull request, we identified @1st1, @fafhrd91 and @serhiy-storchaka to be potential reviewers.

Copy link
Member

Choose a reason for hiding this comment

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

Do we beed to return here?

Copy link
ContributorAuthor

@grzgrzgrz3grzgrzgrz3Jul 5, 2017

Choose a reason for hiding this comment

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

There is no need. When we return - this code is Omitted :
del self._write_backlog[0]
If _process_write_backlog is called after shutdown, we ll do shutdown again. If we want to return here we need to add del either. I think this may complicate this method even more.

Maybe something like that gist, but code with write blocked on a read is not tested, i will suggest to not make more changes.

I am planing to add more unittests in future and refactor sslproto.

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to wrap this in try..finally to guarantee that self._finalize will be called?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

transport will be closed anyway in self._fatal_error, but why not.

Asyncio on shutdown do not send shutdown confirmation to the other side. _SSLPipe after doing unwrap is calling shutdown calback where transport is closed and quit ssldata wont be sent. Having this callback in _SSLPipe feels wrong and its removed. Removing callback from _SSLPipe.shutdown should not break any api.
@1st1
Copy link
Member

1st1 commented May 28, 2018

Could you please rebase the PR against the current master? NEWS should be generated with blurb

@csabella
Copy link
Contributor

It appears that the author of the pull request is no longer active. I'm going to close this PR so someone else can create a new one if they are interested.

@asvetlov
Copy link
Contributor

Agree, I'm working on the better fix here: #17975

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@grzgrzgrz3@mention-bot@1st1@csabella@asvetlov@brettcannon@Mariatta@the-knights-who-say-ni