Add socket messages callbacks and comms API #376
Conversation
jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/libraries/connection.kt
Outdated
Show resolved
Hide resolved
jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/libraries/connection.kt
Outdated
Show resolved
Hide resolved
jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/util/serializers.kt
Outdated
Show resolved
Hide resolved
jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/libraries/connection.kt
Show resolved
Hide resolved
jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/libraries/connection.kt
Outdated
Show resolved
Hide resolved
jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/libraries/connection.kt
Show resolved
Hide resolved
| fun messageReceived(data: JsonObject) { | ||
| if (closed) return | ||
| for (callback in onMessageCallbacks) { | ||
| callback(data) |
There was a problem hiding this comment.
Right now here we execute callbacks on a callback thread inside another callback. I think, it's better to introduce some pool which would do that work for Comms instead of calling them right here.
Since it's callbacks, we don't need to worry much about stack-frame-awareness, but it would save time for the calling thread in case of large computations or a lot of messages
jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/util/serializers.kt
Show resolved
Hide resolved
| override fun <T> runExecution(classLoader: ClassLoader?, body: () -> T): ExecutionResult<T> { | ||
| var execRes: T? = null | ||
| var execException: Throwable? = null | ||
| val execThread = thread(contextClassLoader = classLoader ?: Thread.currentThread().contextClassLoader) { |
There was a problem hiding this comment.
Sorry for posting a bit late. This would work, but creating and stopping a new thread a bit costly since it's OS call.
The more consise would be to reuse existing threads, so they wait for some tasks to appear from a queue and execute them, e.g. pool aproach. So perhaps we might reuse some existing, like Executors.newCachedThreadPool
There was a problem hiding this comment.
Yes, I undestand. It's just how it was implemented before. I will change it in a separate commit
Fixes #366
Should also partially fix the needs of #375