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

Documentation Update #1130

Closed
wants to merge 3 commits into from
Closed

Conversation

blakebjorn
Copy link

Mention that init_app() ignores arguments passed to the SocketIO constructor in the docs - see issue #321

Spent a lot of time troubleshooting everything imaginable before I found out this was the root of the issue

See issue miguelgrinberg#321 - This should be documented as I spent most of a day troubleshooting this
@miguelgrinberg
Copy link
Owner

This is the standard behavior of all Flask extensions. You either pass the args on the main class, or pass none, and then pass them in init_app.

I'm not sure your short statement really helps in clarifying this. If you really wanted to document this, then you would have to show examples of both.

@blakebjorn
Copy link
Author

blakebjorn commented Dec 19, 2019

I agree that anything in the documentation should be more robust, but at the same time it's a fringe case and I don't want to derail the paragraph on that account.

I'm also just realizing that my blurb doesn't even accurately describe the issue - being that the only argument being arbitrarily dropped is the message_queue. (This was brought up in #311 but closed because specifying the arguments in init_app was a simple enough workaround)

e.g.

socketio = SocketIO(app, async_mode="eventlet", message_queue="redis://localhost:6379")

launches the webserver with eventlet and a functional redis backend

socketio = SocketIO(async_mode="eventlet", message_queue="redis://localhost:6379")
socketio.init_app(app)

launches the webserver with eventlet and a non-functional redis backend

I think perhaps a better solution than documenting this would be to just fix the inconsistent behaviour - when the socketio instance is constructed without a functional flask application passed the message queue is instantiated with write_only mode. This obviously makes sense so you can instantiate a socketio instance to emit messages without being bound to the request context (and you don't need to listen to anything client side). HOWEVER if you subsequently call init_app() it reuses the already instantiated write-only client.

The updated PR removes the blurb from the docs and instead modifies init_app to disable write_only on the message client if it has already been created (since the pubsub manager hasn't initialized at this point, unless you are somehow calling init_app() after socketio.run()) - This work fine with Redis, I haven't tested with the other back-ends but I can't see why it wouldn't work the same. If there is any issues I think just throwing a warning here would also be adequate (e.g. "This SocketIO instance has already instantiated a message queue in publish-only mode. To re-instantiate with subscriber capabilities, the message_queue argument needs to be passed in the init_app call")

@shubidubapp
Copy link

shubidubapp commented May 12, 2021

I have wasted too much time because of this. Since everything works except for the message_queue (inconsistently) and in the logs it actually says the redis backend is initialized and no error is thrown I didnt even think about checking the init_app part. I checked a lot of issues, almost every line on documentation and to think that I found my answer to the problem in a pull request titled as "Documentation Update" opened at 2019 and still havent been merged or anything else blows my mind.

Yes this can be standard behavior for Flask Extensions but I am not passing any arguments to most of the extensions while creating the object or in init_app I didn't even know this was the standard behavior.

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

Successfully merging this pull request may close these issues.

None yet

3 participants