-
Notifications
You must be signed in to change notification settings - Fork 74
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
remove Arc around RequestContext #558
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 tasks
7 tasks
…h the same name) add demo test fix up docs a bit
ahl
reviewed
Jan 14, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This depends on #557. Leaving in "draft". This is a prototype.
This change removes the
Arc
aroundRequestContext
in all of the request handler functions. This is something I've been meaning to revisit for a while but it came up while doing #557, etc.On the one hand, I think this is a good change in isolation: nobody else has a reference to this object, and in fact the whole point of this object is to encapsulate for the request handler whatever Dropshot wants to pass to it for this request. So it doesn't make sense for Dropshot to hold its own reference to it. Dropshot can keep copies or references to whatever things inside the RequestContext that it wants. It's simpler for everybody to get rid of this
Arc
and it makes it less likely that people will accidentally hang onto copies of it.The main downside is that this change has a big blast radius for consumers -- literally every endpoint function has to change. But the change is trivial and any mistakes would be caught at compile-time. I used this vim command:
%s/Arc<RequestContext<\([^>]*\)>>/RequestContext<\1>/gc
. Another option would be to explore supporting both forms, but that feels more confusing going forward.I also considered making it
&RequestContext
. I'm not sure what the right lifetime for that would be, if you wanted to make it explicit. And again, my thought was: dropshot has created this object for you, so why not let you have it? In retrospect I wish most of the fields themselves were references (or that we only exposed references). Maybe if we did that and only gave you&RequestContext<'a>
, then we could actually prevent leakage of these fields without you explicitly cloning them. That'd be a bigger change for consumers using these fields.Thoughts? (CC @ahl)