Skip to content

Conversation

nick1udwig
Copy link
Member

Problem

  1. I can receive WS messages other than Close after closing a WS client,
  2. The task is never cleaned up.

Solution

  1. Remove item from map immediately upon a close & use that as sign to not send,
  2. Clean up the task with should_exit flag.

Docs Update

None.

Notes

This is technically a breaking change because it is plausible that a user might have relied on some messages sneaking through. If they did that would be highly unreliable. Good thing this release is a breaking release anyways!

TODO: test using https://github.com/nick1udwig/comfyui_provider which unearthed this issue to begin with

Copy link
Contributor

@dr-frmr dr-frmr left a comment

Choose a reason for hiding this comment

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

LGTM!

@nick1udwig
Copy link
Member Author

Realizing a pretty unintuitive behavior that results from this + our send_and_await semantics.

My expectation was that when I close WS, I should receive an Ack and then the only subsequent WS message I should ever receive should be a Close. Instead, I am observing the receipt of some number of WS content-ful messages in addition to the Close after closing.

What is going on here is:

  1. WS is receiving frequent messages,
  2. Close the WS & await the Ack,
  3. Before Ack arrives, multiple WS content-ful messages have arrived and been sent and are now in my process message queue,
  4. WS is closed, Ack arrives, process unblocks,
  5. Work through queue, including the WS content-ful messages,
  6. No more additional WS content-ful messages are added to queue, but eventually the Close comes in.

This is pretty unintuitive. I don't know that it breaks anything, but it does add some annoying boilerplate I have to add to properly handle the WS useless content-ful messages post close.

@nick1udwig
Copy link
Member Author

I guess its not terrible since you have to handle the Close anyways.

@nick1udwig nick1udwig marked this pull request as ready for review April 22, 2024 16:29
@nick1udwig nick1udwig merged commit 41e59fb into develop Apr 22, 2024
@nick1udwig nick1udwig deleted the hf/ws-client-close-fix branch April 22, 2024 16:30
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