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

boostification of all websocket signals #47899

Merged
merged 6 commits into from
Feb 1, 2024
Merged

Conversation

sima-fastly
Copy link
Contributor

@sima-fastly sima-fastly commented Jan 31, 2024

boostification of all websocket signals

tested manually

@sima-fastly sima-fastly marked this pull request as ready for review January 31, 2024 05:23
@sima-fastly sima-fastly self-assigned this Jan 31, 2024
@sima-fastly sima-fastly changed the title boostification of ws connected signal boostification of all websocket signals Jan 31, 2024
@sima-fastly sima-fastly requested a review from jkarneges January 31, 2024 20:30
Copy link
Contributor Author

@sima-fastly sima-fastly left a comment

Choose a reason for hiding this comment

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

@jkarneges I have updated the code to delete connection struct instace when a req/sock gets deleted.

but this reminds me there are other places such as this file :
https://github.com/fastly/pushpin/blob/adf96ae5692c8c45e6835b40ea4dfb51f588142f/src/cpp/proxy/requestsession.cpp#L238C4-L238C23
where a req or socket are getting deleted without the corresponding connection getting disconnected. Am I correct in thinking these also need a disconnect statement fix?

@sima-fastly sima-fastly requested a review from jkarneges January 31, 2024 21:56
@jkarneges
Copy link
Member

Ah yeah, anywhere an object is deleted that might have had connections, especially if the deletion is done from somewhere that isn't the destructor and the object pointer is also nulled out, any connections should also be disconnected.

I suggest keeping the connections object as a value and not a pointer, or else you'll need to null out the connections object too. The connection object is initially implicitly constructed using its default constructor, so explicitly calling the default constructor later on should return it to its initial state.

@sima-fastly
Copy link
Contributor Author

Ah yeah, anywhere an object is deleted that might have had connections, especially if the deletion is done from somewhere that isn't the destructor and the object pointer is also nulled out, any connections should also be disconnected.

I suggest keeping the connections object as a value and not a pointer, or else you'll need to null out the connections object too. The connection object is initially implicitly constructed using its default constructor, so explicitly calling the default constructor later on should return it to its initial state.

I have addressed this and will have a follow up PR to clean these up in other files

@sima-fastly sima-fastly merged commit 8936b3d into main Feb 1, 2024
1 check passed
@sima-fastly sima-fastly deleted the sima/boost/ws-connected branch February 1, 2024 17:41
sima-fastly added a commit that referenced this pull request Mar 14, 2024
* boostification of ws connected signal

* full boostification of websocket

* addressed PR comments

* disconnect issue addressed

* PR comment addressed
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