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 into
python:masterfrom
jamadden:closing
Closed

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

Conversation

@vstinner

@vstinner vstinner commented Jul 20, 2015

Copy link
Copy Markdown
Member
  • 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

asvetlov commented Jul 21, 2015

Copy link
Copy Markdown

LGTM

gvanrossum
gvanrossum reviewed Jul 21, 2015
View reviewed changes
Comment thread asyncio/streams.py

@gvanrossum gvanrossum Jul 21, 2015

Copy link
Copy Markdown
Member

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

gvanrossum commented Jul 21, 2015

Copy link
Copy Markdown
Member

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

gvanrossum commented Sep 25, 2015

Copy link
Copy Markdown
Member

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

1st1 commented Nov 13, 2015

Copy link
Copy Markdown
Member

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

gvanrossum commented Nov 13, 2015 via email

Copy link
Copy Markdown
Member

@1st1

1st1 commented Nov 13, 2015

Copy link
Copy Markdown
Member

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

gvanrossum commented Nov 13, 2015 via email

Copy link
Copy Markdown
Member

@asvetlov

asvetlov commented Nov 13, 2015

Copy link
Copy Markdown

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

1st1 commented Nov 13, 2015

Copy link
Copy Markdown
Member

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

gvanrossum commented Nov 16, 2015

Copy link
Copy Markdown
Member

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

1st1 commented Nov 16, 2015

Copy link
Copy Markdown
Member

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