Skip to content
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

KTOR-4323: WebSockets use custom serializer #3000

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hfhbd
Copy link
Contributor

@hfhbd hfhbd commented May 15, 2022

Subsystem
Client and Server, Websockets with Kotlinx serialization

Motivation
It is not possible to use WebSockets with a custom KSerializer

Solution
Added a KSerializer as a parameter


import io.ktor.websocket.*

public interface SerializableWebSocketSession : WebSocketSession {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name is misleading. the session itself is not serializable, but has a serializer

Copy link
Contributor Author

@hfhbd hfhbd May 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's true. I didn't found a better name through, maybe WebSocketSessionWithSerializer or WithConverter?


@InternalAPI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this annotation is needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I use writeFrame(Frame.Close()), which I copied from other tests.

client.webSocket("$TEST_WEBSOCKET_SERVER/websockets/echo") {
repeat(TEST_SIZE) {
val originalData = Data("hello")
sendSerialized(originalData, DataSerializer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the use case of passing serializer here? can't it be registered in module or through @UseSerializers annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not work, if you need to serialize 3rd classes without the possibility to change them. @UseSerializers requires an annotation, and the SerializersModule for generic classes requires @contextual annotation too, according to the docs. It is also not possible to serialize Any with the overloads only, but it is with a custom serializer. That's the problem :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think something like this may be useful for regular requests as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, isn't it currently possible? I tried automatic content negation never before, with web sockets it is my first usage :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add it in another PR and focus this PR on Websockets only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: have you tried to register external serializer for 3rd party class via SerializersModule via contextual call?. According to code it should just work by fetching serialiaer by KClass
Or can you at least provide an example of what you want to archive, and it fails

Copy link
Contributor Author

@hfhbd hfhbd Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted my old reproducer and at the moment reproducing the case does not work. But adding all possibilities via contextual or polymorphic to the serializer module is annoying, so passing the serializer directly at usage is nicer IMHO. And the implementation is quite forward for web sockets.

private val json = Json {
    serializersModule = SerializersModule {
        val s = Main.M.serializer(Int.serializer()) as KSerializer<Main.M<*>>
        polymorphic(Main::class, Main.M::class, s)
    }
}

fun main() {
    val main = Main.M(42)
    println(json.encodeToString(main))

    println(json.encodeToString(Main.M.serializer(String.serializer()), Main.M("foo")))
}

@Serializable
sealed class Main {
    @Serializable
    data class M<S>(val value: S) : Main()
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that both ways are little annoying :)
But, even it's quite forward for websockets, looks reasonable to add the same to http endpoints, but there it will be not such easy.

And the main my concern about, why it's not good - calling this method will fail, if there is non kx.serialization converter.
Or f.e. if there will be some custom converter, which delegates to kotlinx.serialization converter, but uses another encoding for some types. So using serializersModule will work in both cases, but providing explicit serializer will work only in case of plain kx.serialization converter - such an inconsistency isn't so good in my opionion.

And in this specific use case, may be it will be much easier to not use this plugin at all, and do serialization handling manually, or write a simple plugin for your use case? :)

Copy link
Contributor

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some more questions


import io.ktor.websocket.*

public interface WebSocketSessionWithContentConverter : WebSocketSession {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: why this interface is needed, for me it's a little strange TBH
I see why it's needed, as converter for client and server are retrieved in different ways.
But IMO, even if add explicit serializers support, better to just add extensions in server and client modules for serialization with explicit serializer parameter as with current (

public suspend inline fun <reified T> DefaultClientWebSocketSession.sendSerialized(data: T) {
and
public suspend inline fun <reified T> WebSocketServerSession.sendSerialized(data: T) {
)

Copy link
Contributor Author

@hfhbd hfhbd Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but the problem is the classpath. KSerializer is defined in kotlinx-serialiation, which is only added as dependency in ktor-shared/ktor-serialization-kotlinx. To create an extension function in the client web sockets it needs to resolve KSerializer. But the actual serialization lib (kotlinx.serialization) is an implementation detail, and not available in the core module (this is the same problem with client.get { body(foo, FooSerializer) etc.)

One workaround: adding kotlinx.serialization core to the dependencies of client-core /server-core

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I see.

Now I think, that may be some additional serialization plugin should be created for kotlinx.serialization specific usage, which will support all it's cool things, like explicit serializers provider, and reduced overhead of usage reflection over typeOf serialization retrieval. But not sure if it's really needed.

Frame.Binary(true, content)
}
else -> error("Unsupported format $format")
}
}
}

public suspend fun <T> WebSocketSessionWithContentConverter.sendSerialized(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO: such tightly coupling of interface to implementation isn't good for general purpose API

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.

None yet

3 participants