Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix Socket's WorkerInterface Lifetime #466

Merged
merged 1 commit into from
Mar 22, 2023
Merged

Conversation

Warfields
Copy link
Contributor

@Warfields Warfields commented Mar 21, 2023

In long lived actors it is possible that the promise provided by connect() can outlive the WorkerInterface that is used to create it's http client. this can lead to unexpected Segfaults when the socket is deconstructed.

The object returned by kj::newHttpClient(HttpService& service) does not take ownership of the underlying HttpService, whereas asHttpClient() does. This way the WorkerInterface providing the HttpClient will live for the length of the promise.

This also done for the equivalent client in the fetch() implementation for WebSockets.

@Warfields Warfields force-pushed the swarfield/socket-lifetimes branch from f8ec874 to 83fab08 Compare March 21, 2023 18:58
In long lived actors it is possible that the promise provided by
`connect()` can outlive the WorkerInterface that is used to create
it's http client. this can lead to unexpected Segfaults when the
socket is deconstructed.

The object returned by `kj::newHttpClient(HttpService& service)` does
not take ownership of the underlying HttpService, whereas asHttpClient()
does. This way the WorkerInterface providing the HttpClient will live
for the length of the promise.

This also done for the equivalent client in the `fetch()` implementation
for WebSockets.
@Warfields Warfields force-pushed the swarfield/socket-lifetimes branch from 83fab08 to c760667 Compare March 21, 2023 19:44
Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Nice!

@Warfields Warfields merged commit 805b61b into main Mar 22, 2023
@fhanau fhanau deleted the swarfield/socket-lifetimes branch September 29, 2024 04:10
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants