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

Enable running on Windows #1192

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

garfieldnate
Copy link

Add Windows to ExternalBuild so that builds on Windows are recognized
and used when appropriate.

Fix WsgiServer, which is used for communicating with the Unity process
on Windows, by pinning to an older version of flask/werkzeug that still
supports TCP keepalive. Some background:

AgentManager.cs creates a single Socket object and communicates
through it for the lifetime of the application. This is different from
the default HTTP request flow, where a TCP connection is created at the
beginning of the request and destroyed at the end. For this connection
to work properly, TCP keepalive must be enabled.

Previously, setting werkzeug.serving.WSGIRequestHandler. protocol_version = "HTTP/1.1" (as WsgiServer does) would activate TCP
keepalive by default. However, the flask/werkzeug built-in server relies
on the python built-in server, which had (has?) a bug in keepalive
connections: if you didn't read the request body, it wouldn't properly
empty the socket, and then the next time an HTTP request handler fired
the handler would read data from the previous request. Because the
server shipped with flask/werkzeug is not meant to be productionizable,
and several users were complaining about this issue, the devs just
disabled keepalive altogether and moved on. Removal note here:
https://werkzeug.palletsprojects.com/en/2.3.x/changes/#version-2-1-2.
More details here: pallets/werkzeug#2397.

I think WsgiServer was implemented with a previous version of flask/
werkzeug, and it broke when the version numbers were updated.

I understand that Windows is not officially supported, but its usage is
a requirement for some, and even just enabling users to build on Windows
themselves will be very useful.

These changes do not affect the FifoServer, which is used on MacOS and
Linux.

These are the last versions where keepalive has not been removed from the
development server with the protocol set to HTTP/1.1. TCP keepalive is required
for the socket communication in AgentManager.cs to work properly, since it only
opens one socket for the lifetime of the application.
@Lucaweihs Lucaweihs self-requested a review April 1, 2024 17:49
Backslashes require another backslash for escaping; otherwise they are
interpreted in Python as escaping the following character, leading to unhelpful
error messages when the unity process exits.
Use Posix-style forward slashes in the Unity executable path on Windows, as
these won't ever be misinterpreted as being escape characters.
@garfieldnate
Copy link
Author

@Lucaweihs I pushed the other needed changes. It's ready for review :) .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants