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.

Sign up for GitHub

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

Jump to bottom

[wptserve] Remove ResponseWriter.flush() and explicit_flush #24719

Closed
Hexcles opened this issue Jul 23, 2020 · 2 comments
Closed

[wptserve] Remove ResponseWriter.flush() and explicit_flush #24719

Hexcles opened this issue Jul 23, 2020 · 2 comments
Labels
infra priority:roadmap python3 wptserve

Comments

@Hexcles
Copy link
Member

Hexcles commented Jul 23, 2020

From #24617 (review):

I'd like to suggest dropping the flush method on ResponseWriter altogether to prevent any future confusion.

Digging through stdlib, I don't think there's any need to call flush ever. In both Python 2 and 3, the base HTTP server inherits from SocketServer.StreamRequestHandler with the default wbufsize = 0.

In both versions, wfile.write() ultimately calls sock.sendall which either succeeds sending all data or raises an exception: https://docs.python.org/3/library/socket.html#socket.socket.sendall

So basically, the reliable way to detect aliveness of a socket is to check wfile.write(). flush() is either not necessary (in Python 2) or non-functional (in Python 3), and should be removed for clarity.
This was referenced Jul 23, 2020
Merged
Merged
@Hexcles
Copy link
Member Author

Hexcles commented Jul 23, 2020

We'd need to

  1. Add return statements to a few write* methods to surface caught errors from _wfile.write.
  2. Change handlers to check the return values of write instead of flush. And drop the use of flush or explicit_flush.
  3. Remove flush/explicit_flush from Response.

Hexcles pushed a commit that referenced this issue Jul 27, 2020
@ziransun
Add return statement for ResponseWriter.write_content() function. (#2…
057105b 
…4716)

Related discussions:
#24617
#24657

Part of #24719
Hexcles pushed a commit that referenced this issue Jul 28, 2020
@ziransun
Python 3: Drop use of flush on ResponseWriter in a couple of files (#…
a5583f1 
…24657)

This patch only drops the use of response.write.flush() in a couple of
files as they directly affect test results in Python 3.

Detailed discussions on dropping use of flush on ResponseWriter can be
found at #24617
(Also see #24719 )
@foolip
Copy link
Member

foolip commented Sep 22, 2020

@Hexcles do you plan to work on this yourself? It's labeled roadmap priority, but with nobody assigned it seems unlikely anything will happen :) Perhaps it was already fixed by some of the PRs that landed in July?

ziransun added a commit to ziransun/wpt that referenced this issue Nov 12, 2020
@ziransun
Remove ResponseWriter.flush() and explicit_flush
05a2991 
The reliable way to detect aliveness of a socket is to check
wfile.write(). flush() is either not necessary (in Python 2)
or non-functional (in Python 3), and should be removed for clarity.

Detailed explaination can be found at:
web-platform-tests#24719
@ziransun ziransun mentioned this issue Nov 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
infra priority:roadmap python3 wptserve
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@foolip @Hexcles