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.
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
Fix
EventLoopFuture
andEventLoopPromise
under strict concurrency checking #2654Fix
EventLoopFuture
andEventLoopPromise
under strict concurrency checking #2654Changes from 15 commits
e0a87fc
5d20621
3ae701c
aeb2634
469e475
426e453
7f83110
b191621
b9b9ea6
77da82c
bfe84d2
75ad4d7
28029bf
dacc452
c41d45d
7ce1c36
d0efcf3
4252ecc
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.
assumeIsolated()
is a noop inrelease
, right?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.
Right now it is but it needs to change. It uses
assert
s to check but we need to upgrade thispreconditions
since check at creation time is not enough similar to loop bound. Here is an example where just aprecondition
on creation of theassumeIsolated
would failThere 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.
@FranzBusch the line above is
if self.eventLoop.inEventLoop()
, so theassumeIsolated()
is duplicated.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.
Ah sorry, I thought you were asking in a more general term. In this particular case I don't know if the compiler is able to optimise the precondition away.
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.
No, it doesn't do that sort of thing. If we're not sure that this is a noop, we should remove it here. Already not super cheap to check it
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.
Or do
assertIsolated
notpreconditionIsolated
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.
Probably a nit but I'm not sure if
Task
is 100% correct here. Like there's no way to retrieve alet theTask: Task
from a child task. But then, child tasks still askTask.isCancelled
so maybe it is correct :).I would've said that they can run Jobs but then that's not really terminology that's actually used. Probably fine
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.
I tried for a while to rewrite this and couldn't come up with anything I liked. If we think of anything better we can change it.
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.
Not sure the repeated exact Note as above here isn't too confusing, maybe explicitly call out what "these" are or shorten the repeated notes?
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.
I think that's right, yes.