Skip to content
This repository was archived by the owner on Nov 23, 2017. It is now read-only.

Issue #248: Add closing read-only property to transports#257

Closed
vstinner wants to merge 1 commit intopython:masterfrom
jamadden:closing
Closed

Issue #248: Add closing read-only property to transports#257
vstinner wants to merge 1 commit intopython:masterfrom
jamadden:closing

Conversation

@vstinner
Copy link
Copy Markdown
Member

@vstinner vstinner commented Jul 20, 2015

  • Disallow write() on closing transports
  • Disallow aslo calling pause_writing() and resume_writing() on StreamReaderProtocol if the transport is closing

@vstinner
Add closing read-only property to transports
13b71a1 
* Disallow write() on closing transports
* Disallow aslo calling pause_writing() and resume_writing() on
  StreamReaderProtocol if the transport is closing
@vstinner vstinner mentioned this pull request Jul 20, 2015
Closed
@asvetlov
Copy link
Copy Markdown

asvetlov commented Jul 21, 2015

LGTM

gvanrossum
gvanrossum reviewed Jul 21, 2015
View reviewed changes
asyncio/streams.py
Copy link
Copy Markdown
Member

@gvanrossum gvanrossum Jul 21, 2015

Choose a reason for hiding this comment

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

I think you should call super().connection_made(transport) here.

@gvanrossum
Copy link
Copy Markdown
Member

gvanrossum commented Jul 21, 2015

So honestly this worries me, so please don't commit yet. FWIW I do think it's fine if closing remains set after the transport is closed.

@pohmelie pohmelie mentioned this pull request Sep 19, 2015
Closed
@gvanrossum
Copy link
Copy Markdown
Member

gvanrossum commented Sep 25, 2015

I really don't like that write() can raise an exception before connection_lost() has been called. It feels like a violation of the state machine specification. But exposing the closing flag is fine. Can you try a patch with just that?

@1st1
Copy link
Copy Markdown
Member

1st1 commented Nov 13, 2015

I was hit by this too -- I need to implement transport._closing in uvloop. It's a private API, but it's used a lot in asyncio and in third-party code. Can we add a simple method Transport.is_closing() for this?

@gvanrossum
Copy link
Copy Markdown
Member

gvanrossum commented Nov 13, 2015 via email

@1st1
Copy link
Copy Markdown
Member

1st1 commented Nov 13, 2015

Transport.is_closing() is more consistent with Loop.is_running() and Loop.is_closed().

And with a quick regex search def \w+ing\( I couldn't find methods like .something().

@gvanrossum
Copy link
Copy Markdown
Member

gvanrossum commented Nov 13, 2015 via email

@asvetlov
Copy link
Copy Markdown

asvetlov commented Nov 13, 2015

Sorry for offtopic but would you, guys, take a look on http://bugs.python.org/issue25074
Approving or rejecting of proposed patch would be great.

@1st1
Copy link
Copy Markdown
Member

1st1 commented Nov 13, 2015

Sorry for offtopic but would you, guys, take a look on http://bugs.python.org/issue25074
Approving or rejecting of proposed patch would be great.

If nobody looks at it before Monday I can review it.

@gvanrossum
Copy link
Copy Markdown
Member

gvanrossum commented Nov 16, 2015

So, I think we need a new PR that *just * adds is_closing() to the
Transport base class and to all its implementation subclasses. Who wants to
do that? We have until Friday to get it into 3.5.1.

@1st1
Copy link
Copy Markdown
Member

1st1 commented Nov 16, 2015

Please see PR #291. Closing this one.

@1st1 1st1 closed this Nov 16, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vstinner @asvetlov @gvanrossum @1st1