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

fix(gloo-net): remove bug-abling EventSource Clone implementation #417

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vpochapuis
Copy link
Contributor

PR Content

Remove the Clone derive/impl for the EventSource .

Reasoning

After discussing #409, it seems that the implementation of Clone for EventSource is a bug.

To summarize the situation:

EventSource implements Drop and will disconnect the internal connection upon triggering this mechanism.
However, when cloning this structure, if any instance of EventSource is going out of scope, all of the instances will have their internal connection disconnected.

An example of a situation where this can be unexpected to the user and not straight forward:

  • Using yewdux to manage a mutable global state. yewdux's state will need to have the structs field that impl Clone. Hence here using EventSource in its previous version, would be allowed, without any additional wrapper. However, internally yewdux clone's and drops the state, hence destroying the precious connection and rendering the EventSource virtually unusable, but without any explanation to the user.

Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Can you also update CHANGELOG.md accordingly

@vpochapuis
Copy link
Contributor Author

vpochapuis commented Dec 2, 2023

Thanks for the PR! Can you also update CHANGELOG.md accordingly

I will do my best to follow what is in CONTRIBUTING.md however I do not understand well how it works, as the CHANGELOG.md says that net is at version 0.5.0 while the Cargo.toml of net is at 0.4.0.

Please let me know if this is correct and how I could improve for the next time.

Thank you!

@vpochapuis vpochapuis requested a review from ranile December 2, 2023 15:11
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