-
Notifications
You must be signed in to change notification settings - Fork 278
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
Changing a method of lift actor. #1940
Conversation
@@ -171,7 +171,7 @@ class LAFuture[T](val scheduler: LAScheduler = LAScheduler, context: Box[LAFutur | |||
/** | |||
* Has the future been aborted | |||
*/ | |||
def aborted_? = synchronized {satisfied} | |||
def aborted_? = synchronized {aborted} |
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.
Oh my. This.... is a pretty bad bug. 🤔
@lift/committers Thoughts on incorporating this fix back to 3.0 since this certainly isn't the intended behavior?
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.
Oh wait, we... we never actually flip aborted
to true as far as I can see, either. It looks like we should do that in abort()
yeah?
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.
We do, in the fail
method, which abort
also falls.
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.
So, re: backporting this, the problem is it really changes a fundamental behavior… By literally flipping the previous behavior <_<
But, the existing behavior is so wrong that it might still be worth it? This is a tough call.
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.
Yeah I'm over here watching and eating popcorn cuz I ain't got a clue.
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.
Per our support doc, ‘API-incompatible changes will only ship as a part of a major release’… This… is pretty API-incompatible…
So perhaps the big question isn't “do we backport this to Lift 3.0” but… Do we merge it into Lift 3.3 at all?
My feeling is yes to both… But it's a pretty fuzzy feeling.
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'm leaning toward "merging for Lift 3.3, not backporting, and including BIG BOLD TEXT in the release notes for 3.3" - mostly because this never worked as intended. I'd also love a test to make sure we don't do something foolish like this in the future. :(
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.
+1 not backporting 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.
👍 I'm down with that.
Re: something foolish like this, thus code review :)
Jut realized the committers group includes a lot of folks who are inactive 🤔. Sorry folks, was in my github flow and didn't think about it. May be worth having a committers emeritus group or something for folk that are inactive. |
@zhongsigang Thanks for the contribution, sorry it's taken us a bit to figure out how to handle it. Would you be up for creating / updating some unit tests for LAFuture to ensure this regression doesn't accidentally reappear in the future? If not, we can handle it out of band. |
Unfortunately, I developed it on windows, and it's not easy to build a test
environment.
Thanks.
2018-03-04 1:21 GMT+08:00 Matt Farmer <notifications@github.com>:
… @zhongsigang <https://github.com/zhongsigang> Thanks for the
contribution, sorry it's taken us a bit to figure out how to handle it.
Would you be up for creating / updating some unit tests for LAFuture to
ensure this regression doesn't accidentally reappear in the future? If not,
we can handle it out of band.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1940 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AOi_Lpu5-JJwFjFhMbHXZoD6As-GuTXDks5tatEhgaJpZM4R_bvY>
.
|
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.
Officially giving this the seal of approval. Will merge in a few days barring objections.
An actor method was implemented incorrectly.