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

Socket fixes and improvements #485

Merged
merged 14 commits into from Feb 10, 2023
Merged

Socket fixes and improvements #485

merged 14 commits into from Feb 10, 2023

Conversation

thavocado
Copy link
Contributor

@thavocado thavocado commented Feb 9, 2023

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Description of changes:

Users can now select Socket.IO transport methods

A new option has been added to pcconfig.py pc.Config

backend_transports=pc.Transports.POLLING_WEBSOCKET

There are 4 options:

  1. pc.Transports.POLLING_WEBSOCKET – the default, initially connect to the back end using http/s requests (long polling) and upgrade to websockets
  2. pc.Transports.WEBSOCKET_POLLING – connect using websockets outright, and fall back to long polling if they're blocked
  3. pc.Transports.WEBSOCKET_ONLY – connect using websockets only, often faster
  4. pc.Transports.POLLING_ONLY – connect using long polling only (don't use websockets)

Additional configuration options for backend connections

cors_allowed_origins=["*"] – List of origins that are allowed to connect to the backend API. It will be useful to set this to a non-catchall on production servers.

Example usage when on a production server: cors_allowed_origins=["https://website.com", "https://myothersite.com"],

cors_credentials=True – Whether credentials (cookies, authentication) are allowed in requests to the backend API.

polling_max_http_buffer_size=1000*1000 – The maximum size of a message when using the polling backend transport in bytes. Defaults to 1MB. This is useful to increase if you want to send images and bigger data sets using long polling.

New state getters which return session id, client IP and client headers

This is useful for various reasons such as wanting to control API access or rates by IP, getting the visitor's browser, language settings, etc.

Example usage:

class State(pc.State):
    """The app state."""

    @pc.var
    def sid(self):
        # session id
        return self.get_sid()

    @pc.var
    def ip(self):
        # client IP
        return self.get_client_ip()

    @pc.var
    def headers_ua(self):
        # browser user-agent, always check whether the key exists
        d, k, v = self.get_headers(), 'user-agent', ''
        if k in d:
            v = d[k]
        return v

Called using

pc.text(
    State.sid,
),
pc.text(
    State.headers_ua,
),
pc.text(
    State.ip,
),

Note that these default to reasonable blanks. These variables only return useful information upon firing of the first event, which usually happens on page load anyway. It's still important to check these have been obtained before using them.

Fixes

Closes #481.

The socket.io app was previously mounted on / which caused problems with dynamic routes. It has now been moved to /event proper. No server re-configuration is necessary as the API endpoint remains the same. Initially I thought this wasn't doable but it's possible to set the socket.io path to "".

Tested locally and a production server.

@thavocado
Copy link
Contributor Author

thavocado commented Feb 9, 2023

I see there's issues with how the transports defaults are pulled out. I'm not exactly sure what the best option is for lists of strings, but I can simply make the option a string and that will fix the errors. Let me know.

Edit: actually that's what I'll do. The additional complexity isn't necessary seeing as this is a JS string list to begin with.

@thavocado
Copy link
Contributor Author

Needs more work to get the CORS config be reasonable. Currently it can be either a string, an empty list, or a list of strings.

@thavocado
Copy link
Contributor Author

Ok should be done now, let me know your thoughts.

@Alek99
Copy link
Contributor

Alek99 commented Feb 10, 2023

Are you getting this error:
RuntimeError: Expected ASGI message 'http.response.start', but got 'websocket.accept'.

Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Nice! Just some nits and I think this needs a rebase

pynecone/app.py Outdated
@@ -35,6 +35,9 @@ class App(Base):
# The Socket.IO AsyncServer.
sio: AsyncServer = None

# The socket app.
socket_app: ASGIApp = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Type should be Optional[ASGIApp]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Done.

pynecone/app.py Outdated
@@ -47,7 +50,7 @@ class App(Base):
# Middleware to add to the app.
middleware: List[Middleware] = []

# events handlers to trigger when a page load
# Events handlers to trigger when a page loads.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Event handlers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@thavocado
Copy link
Contributor Author

thavocado commented Feb 10, 2023

Are you getting this error:
RuntimeError: Expected ASGI message 'http.response.start', but got 'websocket.accept'.

I’ve gotten the opposite error before. I did a bunch of investigation and while it looks like an error caused by the back end, it’s actually an error triggered from the browser-end.

It happened to me when I had an old window open which was trying to connect to the websocket interface using old front end code. Why that should cause such a backend error is a uvicorn mystery.

I’m not 100% sure this is the cause in your case, but try closing old windows and tabs with Pynecone or refreshing them. I suspect the cause is the old Pynecone front end socket code trying to connect.

I did that and haven’t seen this type of error again.

@Alek99 Alek99 self-requested a review February 10, 2023 04:04
@Alek99
Copy link
Contributor

Alek99 commented Feb 10, 2023

Wierd I exited out of all browsers and still seems to be getting it, I think @picklelo is getting it to

@Alek99
Copy link
Contributor

Alek99 commented Feb 10, 2023

I'll look into it rn

Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

Awesome, the errors are all gone now!

@picklelo picklelo merged commit 8d9c758 into reflex-dev:main Feb 10, 2023
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.

Fix api routes
3 participants