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

Remove requirement that job of the pre-created JobCancelNode have to … #2427

Merged
merged 3 commits into from
Dec 7, 2020

Conversation

qwwdfsad
Copy link
Contributor

@qwwdfsad qwwdfsad commented Dec 3, 2020

…be equal to outer job

Job is supposed to be "sealed" interface, but we also have non-sealed Deferred that can be successfully implemented via delegation and such delegation may break code (if assertions are enabled!) in a very subtle ways.

This assertion is our internal invariant that we're preserving anyway, so it's worth to lift it to simplify life of our users in the future

Fixes #2423

…be equal to outer job

Job is supposed to be "sealed" interface, but we also have non-sealed Deferred that can be successfully implemented via delegation and such delegation may break code (if assertions are enabled!) in a very subtle ways.

This assertion is our internal invariant that we're preserving anyway, so it's worth to lift it to simplify life of our users in the future

Fixes #2423
Copy link
Contributor

@elizarov elizarov left a comment

Choose a reason for hiding this comment

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

It fails in case of cancellation of such a delegate (due to unexpected value of JobNode.job which is supposed to extend JobSupport). I've added the corresponding test and a fix. Now looks good!

@elizarov
Copy link
Contributor

elizarov commented Dec 4, 2020

My first attempt to fix it did not work out on JS. Now I've taken a different approach which also simplifies some code around JobNode. Please, review.

Copy link
Contributor Author

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Nice catch and good riddance

@qwwdfsad qwwdfsad merged commit 1176267 into develop Dec 7, 2020
@qwwdfsad qwwdfsad deleted the fix-delegation branch December 7, 2020 11:53
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.

None yet

2 participants