Skip to content

Conversation

@SaintMercury
Copy link

Updated and rebased version of the following PR

#123

Adds mutexes to lua api and fixes the tests (on windows)

@mihacooper
Copy link
Member

As far, as I remember there is a problem in how we implemented a thread cancellation - using exceptions. The problem appeared when I tried to add one more additional test: check what will happen if we cancel a thread hanged on mutex (inside callback). It causes invalid behaviour (SEGFAULT if I remember it right).

So, first of all we need to change the implementation of thread cancellation.

@SaintMercury
Copy link
Author

Ah, I understand. I shall try to test when I can.

Do you propose a total refactor? I noticed on windows that the LuaCancelException thrown in threading.cpp never actually seems to get caught.

@mihacooper
Copy link
Member

yep, we need to do something with current thread cancellation implementation.
I see only two ways: get rid of cancellation functionality or implement it using normal exceptions(lua error()) but in that case cancel error can be catched by pcall method

@SaintMercury
Copy link
Author

SaintMercury commented Nov 10, 2022

After doing some lite research into how other languages handle thread cancellation.

Java deprecated the ability to cancel threads, and Python does not as well.

C# allows this via throwing an exception in the thread that can be caught.

Golang and Rust don't support it either.

It looks like most places recommend passing in some signal channel and handling the cancellation/termination based upon that.

I'd propose probably just disallowing the cancel behaviour natively, and having users design ways to cancel threads.

@mihacooper
Copy link
Member

I have decided to left cancellation behaviour for right now, but implemented it using regular lua error: #177

After this PR will be merged, I will commit mutex PR. So, now I close this PR.

@mihacooper mihacooper closed this Nov 16, 2022
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.

2 participants