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

Implement missing Worker::Isolate::SubrequestClient::connect. #233

Merged
merged 1 commit into from
Dec 21, 2022

Conversation

dom96
Copy link
Collaborator

@dom96 dom96 commented Dec 20, 2022

When testing via fiddle this code path was hit.

@kentonv should connect be implemented in all classes which extend WorkerInterface/HttpService? if not are there any other connect codepaths which are likely to be hit in production and so should be implemented?

@dom96 dom96 requested a review from kentonv December 20, 2022 15:05
@kentonv
Copy link
Member

kentonv commented Dec 20, 2022

should connect be implemented in all classes which extend WorkerInterface/HttpService?

Yes, probably. I wonder if you could redeclare the method pure-virtual at the WorkerInterface level without doing so at the HttpService level, and thus force all subclasses of WorkerInterface to implement it?

@dom96 dom96 force-pushed the dominik/implement-missing-subrequest-connect branch from c0cb7ed to 2c26468 Compare December 21, 2022 10:03
@dom96 dom96 merged commit ca70da3 into main Dec 21, 2022
@kentonv kentonv deleted the dominik/implement-missing-subrequest-connect branch December 21, 2022 15:25
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