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.
add initialization context #126
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
add initialization context #126
Changes from 4 commits
b89f220
39f2241
93bc100
28be2ee
fe320d2
38d7088
f21f4a3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
also considered
InitContext
but this feels more correct since the function is calledinitialize
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.
just added a comment (it was already hopping) the diff is mostly formatting
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.
👀
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.
@tomerd The only situation in which we would need to
hop
is if developers bring their ownEventLoopGroup
. Is that something that is likely?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.
this can happen is if they use some library (lets say a DB client) that manages it's own ELG for whatever reason and they return a future that originated from that ELG. while this is not a pattern we encourage, we also can't stop anyone from doing it so to be safe we should hop imo
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.
Vapor doesn't do a
hop
in its routes and quite a number of people hit that issue at some point. After that they know its an issue and don't do it again. There is an argument for education here instead of protecting against. But at the same time ahop
onto the sameEventLoop
is hardly a performance hit.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.
Just as a reference: Where ever NIO takes a future (not that many APIs outside of bootstraps), it'll
hop
it to the correct event loop.