search

lizmat / Method-Also

Method::Also - add "is also" trait to Methods
https://raku.land/zef:lizmat/Method::Also
Artistic License 2.0
2 stars 2 forks source link

Proposed fix to Proto aliasing issue #2

Closed Xliff closed 5 years ago

Xliff commented 5 years ago

Awaiting review.

lizmat commented 5 years ago

Thanks for this. But it still feels like it is fixing a problem that is actually a bug in class composition of Rakudo itself. Also, it looks to me that the code is not thread-safe, using "global" hashes.

I'm pretty tired at the moment. Your PR is definitely food for thought on researching the class composition in Rakudo.

Xliff commented 5 years ago

Yes, this was pointed out to me. I am reworking some of it.

Talk to you soon.

On Mon, Oct 7, 2019, 2:06 PM Elizabeth Mattijsen notifications@github.com wrote:

Thanks for this. But it still feels like it is fixing a problem that is actually a bug in class composition of Rakudo itself. Also, it looks to me that the code is not thread-safe, using "global" hashes.

I'm pretty tired at the moment. Your PR is definitely food for thought on researching the class composition in Rakudo.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lizmat/Method-Also/pull/2?email_source=notifications&email_token=AAEU5QQTJDTAW4Y7TF3CGM3QNN3DLA5CNFSM4I6BNSZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEARIZZI#issuecomment-539135205, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEU5QWAVVEHZHNT7ERWF6LQNN3DLANCNFSM4I6BNSZA .

vrurg commented 5 years ago

Thanks for this. But it still feels like it is fixing a problem that is actually a bug in class composition of Rakudo itself. Also, it looks to me that the code is not thread-safe, using "global" hashes.

As I partially participated in this fix, I'd note that there is no race condition in the code. It contains nothing that would be run-time executed. Compile-time is single-threaded anyway. And I don't foresee any plans of changing this.

lizmat commented 5 years ago

Multiple threads could do a string EVAL, which both could have a string with "is also" in it.

Unlikely? Perhaps. But nonetheless possible. Or am I missing something?

vrurg commented 5 years ago

Possible, but not now. Eval itself isn't thread-safe. It reuses same compiler instance. Though when I experimented with creating a new one it didn't work too.

Though in general I agree with you.

lizmat commented 5 years ago

EVAL not being thread-safe, is that a known issue?

vrurg commented 5 years ago

Yes. I'm now on my iPad, not easy to find the ticket. But as I remember, it exists.

vrurg commented 5 years ago

Ok, found it: rakudo/rakudo#1391

lizmat commented 5 years ago

Still think it would need to be fixed in core. Until then, this fix will do.