feat: alter clipboard thread to move all clipboard loading from main thread #2867
Conversation
|
Currently untested |
| } | ||
| return bukkitPlayer; | ||
| }; | ||
| TaskManager.taskManager().sync(task); |
There was a problem hiding this comment.
This can cause deadlocks when the server thread is waiting for the current thread. What's the intention behind it?
There was a problem hiding this comment.
The task manager method here will run the given task immediately if it's on the main thread
There was a problem hiding this comment.
Yes but it doesn't know if the main thread is blocked by the current thread
| @Override | ||
| public Thread newThread(@Nonnull Runnable r) { | ||
| return defaultFactory.newThread(() -> { | ||
| IS_UUID_KEYED_EXECUTOR_THREAD.set(true); |
There was a problem hiding this comment.
What's the idea here? I'd assume every thread in this thread pool is always a uuid keyed executor thread? Would it be enough to subtype Thread and do an instanceof check where needed?
There was a problem hiding this comment.
It needs to be accessible from runners within the thread itself. I didn't want to offload to an async thread from main thread needlessly to then run on this thread again. I suppose checking if the current thread is an instance of subtype would be cleaner
|
|
||
| @Override | ||
| public void loadClipboardFromDisk() { | ||
| if (clipboardLoading.isWriteLocked()) { |
There was a problem hiding this comment.
It looks like we only use the write lock. Can't we use a different lock then (or a Semaphore if you want to unlock from a different thread)? Besides that, I think there might be a race condition where the write lock is not locked at this call but below the if. If that happens on the main thread, we stall the main thread.
dordsor21
force-pushed
the
feat/alter-clipboard-thread-usage
branch
from
203948d
to
a71c3b4
Compare
dordsor21
force-pushed
the
feat/alter-clipboard-thread-usage
branch
from
a71c3b4
to
e6481ee
Compare
dordsor21
force-pushed
the
feat/alter-clipboard-thread-usage
branch
from
7ab49c8
to
e1bd063
Compare
| CompletableFuture.completedFuture(callable.call()); | ||
| } catch (Throwable t) { | ||
| CompletableFuture.failedFuture(t); |
There was a problem hiding this comment.
Those are missing returns, causing a deadlock when joining - maybe the the return below should be in an explicit else branch?
No description provided.