Improvements for on method & tests

[maca88]

This PR contains the following improvements:

• Added inline functions for on methods with a callback for simpler usage (e.g. on("add", myMethod))
• Added on overload with a callback and no parameters
• Added error handling for on methods with a callback and no return value (now exceptions will not stop executing other handlers)
• Ported tests for on methods from the Java library
• Fixed logic for sending completion error in case of missing handler with a result
• Updated all on methods to take a suspend callback

[lepicekmichal]

Wow, great work, really appreciated! Thank you!
Unfortunately, we were working on the same thing and now we have conflicts.
However, your pull request contains much more than just client returns. Please, keep it open and I'll look at it and merge it after some minor adjustments.

[maca88]

Huh you are really fast :) Thank you for implementing the requested feature, I didn't expect that you would implement it so quickly.

I did had a look at your changes and I have some points that I would like to point out:

1. With version 0.7.0, it is not possible to register more than one handler without a return value. Example:

val action: (Double) -> Unit = { }
val job = launch { hubConnection.on("add", Double::class, action) }
val job2 = launch { hubConnection.on("add", Double::class, action) /* this call throws an exception */ } 

This is most likely done unintentionally as on overloads without a result type are calling on overloads with the resultType that use hasResult = true, which do check for duplicates.

2. I see that you choose on name also for the handlers with a return value, but there are two limitations with this approach:

• Not possible to add inline functions for both types of handlers as it produces a compile time error, saying that the signature is the same in JVM. The following inline functions produce the same signature in JVM:

suspend inline fun <reified T, reified T1> on(target: String, noinline callback: (T1) -> T) where T: Any, T1: Any =
        on(target, T::class, T1::class, callback)
		
suspend inline fun <reified T1> on(target: String, noinline callback: (T1) -> Unit) where T1: Any =
        on(target, T1::class, callback)

With inline functions it would be possible to do something like this:

hubConnection.on("updateUser", ::updateUser)

private fun updateUser(id: Int, name: String, surname: String): Boolean {
  // do stuff
  return true
}

• Not possible to use the short syntax for callbacks with zero or one parameter. Example for a return handler:

hubConnection.on("inc", Int::class) { 1 }

the above code does not compile as the compiler doesn't know which overload to use. In order to solve the issue, we need to change it to the following to return an integer:

hubConnection.on("inc", Int::class) { -> 1 }
// or
hubConnection.on("inc", resultType = Int::class) { 1 }

Similar is for callback with one parameter and no return value. Example that does not compile do to ambiguity:

hubConnection.on("print", String::class) { println(it) }

the corrected version that works:

hubConnection.on("print", String::class) { p -> println(p) }
// or
hubConnection.on("print", param1 = String::class) { println(it) }

Both limitations are only syntax related, so not important for me. If you are aware of these limitations and are fine for you, then ignore this point.

3. Not able to use suspend methods in handlers with a return value.

This one is important for me, as I do have scenarios where I need to return the result from a suspend function, and I don't want to use runBlocking as it defeats all the benefits of suspend functions.
Here is see two solutions on how to solve the issue:

1. Change the callback to be a suspend function, similar to what I did in this PR
2. Add an additional parameter to the callback (e.g. compleatable), which has to be used for returning a result. Example:

hubConnection.on("doWork", Int::class) { completable ->
  launch {
    val result = suspendFunction()
    completable.setResult(result)
  }
}

For me it doesn't matter which approach is implemented, the most important thing is that we have the ability to return results from suspend functions without having to block the current thread.
Please let me know your thoughts on this one, I am happy to assist if you need help.

[maca88]

I've resolved the conflicts and applied the fix for the first point (register more than one handler without a return value).
Regarding inline functions, it turns out that it is possible to have them by defining only for overloads with a result type.

[lepicekmichal]

Yesterday I published a quick hotfix regarding your 1. The rest I'm thinking I'll do properly tomorrow.
Quick replies:

1. yes, typo
2. I didn't want to have different names right from the start just because official library has it that way. Even though I knew I might come across the same JVM signature in the future. Lemme think about it deeply to resolve this, or make a breaking change in naming.
3. Great feedback, you are absolutely right. I think I'll refactor the whole 'on' part a bit tomorrow.

[maca88]

I merged the latest master and updated the title and description to reflect the current changes located in this PR.

Lemme think about it deeply to resolve this, or make a breaking change in naming.

There is no need to use onWithResult, it is possible to achieve the same by using the on method only. You can check the changes in this PR.

[lepicekmichal]

There is no need to use onWithResult, it is possible to achieve the same by using the on method only. You can check the changes in this PR.

Well, yes and no. If you look at your changes, you'll notice there is one inline function missing. The one without any parameters and that one will be conflicting on JVM.

It is just one, but I could argue it is the most important one. The one that is reacting on server's ask with no parameters needed - as in "hey you still there? yes."

Hence why I have to think of a way out of it or just introduce onWithResult.

[maca88]

If you look at your changes, you'll notice there is one inline function missing

Ups, you are right. I fixed that with the latest commit. As you can see, the solution was just to remove the one that I've added with an inline function instead, which covers both return and non return values.

Ignore the above comment, I've reverted the change as it does not work due to ambiguity, what a pity.

UPDATE: The only solution that seems to work, is by adding the following inline functions outside the HubCommunication class:

suspend inline fun <reified T> HubCommunication.on(target: String, noinline callback: suspend () -> T) where T: Any =
    on(target, T::class, callback)
suspend inline fun <reified T> HubCommunication.on(target: String, noinline callback: () -> T) where T: Any =
    on(target, T::class, callback)

then the following code works fine:

var test: () -> Unit = { }
var test2: suspend () -> Unit = { }
var test3: () -> Int = { 5 }
var test4: suspend () -> Int = { 5 }

hubConnection.on("inc", test)
hubConnection.on("inc", test2)
hubConnection.on("inc", test3)
hubConnection.on("inc", test4)

but you have to import the on inline functions by adding import eu.lepicekmichal.signalrkore.on. I leave this up to you, for me is fine either way.

[lepicekmichal]

I have decided to go for onWithResult naming. Mainly for reasons:

1. Drop-in replacement with signalR official library in java, fewer changes
2. The extensions seem to be fine, but it might not be enough in future.. plus they would be needlessly static
3. Less error-prone.
   For example, when received messages are saved to a list, the add function usually returns boolean for success/failure, that would automatically make the "on" function to be one with result and coder wouldn't necessary see why.
   Especially with dynamic registering of "on" functions listening to the same signalR target.

I'll push a new version over this weekend.

P.S.: Back to your "suspend callback problem". I fully intend to make it suspend, (already made commits of bigger refactoring), but is there a reason not to use the "Flow" variant since you obviously are well-versed in coroutines world? I might make more adjustments if there are some usecases I am not aware of.

[maca88]

but is there a reason not to use the "Flow" variant since you obviously are well-versed in coroutines world?

With the Flow variant, it is not possible to respond back to the server because two pieces are missing:

1. No information about the received hub message, invocationId being the most important missing information
2. There is no way to send a HubMessage back to the server with the invocationId value without using reflection (calling protected complete method)

Here is an example with the Flow variant, where you can better see the missing pieces:

val mySuspendFunction: suspend (String) -> Int = { 10 }

hubConnection.on("Test", param1 = String::class)
  .collect { it ->
    val result = try {
      mySuspendFunction(it)
    } catch (ex: Exception) {
      return complete( // missing complete function
        HubMessage.Completion.Error(
          invocationId = invocationId, // not available
          error = ex.message.orEmpty(),
        )
      )
    }

    complete( // missing complete function
      HubMessage.Completion.Resulted(
        invocationId = invocationId, // not available
        result = Json.encodeToJsonElement(Int::class.serializer(), result),
      )
    )
  }

I might make more adjustments if there are some usecases I am not aware of.

In my opinion, there is no need to extend the Flow variant to support client results if calling suspend methods will be supported with the callback variant. Client results requires to have exactly one handler for each target, which means that most Flow operations (e.g. filtering) are not fit for such cases.

[lepicekmichal]

It took a bit longer but 0.8.0 is published.

As for my previous comment, I went back on it. Instead, I preferred consistency.
All calls to on with callback are returning a result (even if just Unit).
If someone doesn't want to return the result then they should use on returning Flow.

As for conflicting overloads, they got suffixed on JVM (onWithResult and invokeWithResult)

Please let me know if the current version is to your liking.

As for your pull request, there are still tests left. I was looking at them and I am unsure of how much they are copied/inspired by official java library. Are they? I am trying to avoid using their code as much as possible, therefore reluctant to accept the PR.

[maca88]

Thanks for the new release, overall looks very good!

I had to do some changes when merging the latest changes as some tests did fail. It would be nice if these changes would be included in a future release to better match the official client behavior. Let me explain them one by one:

1. Adding the ability to use the on method with a callback also for Unit result type, for example:

hubConnection.on("test", resultType = Unit::class) { }

with the above registration we gain the following benefits in comparison of using the Flow variant:

• Exceptions are automatically handled, there is no need to do it manually by using the Flow<T>.catch method
• There is no need to launch a new coroutine and manually cancel it when stopping the connection. The above method will automatically launch a new coroutine and will stop collecting when HubConnection.stop is called
• The above method is more inline with official SignalR clients (Java, .NET, Javascript) and will provide an easier migration process for developers that would want to switch

2. Move the logic for sending Client did not provide a result. error before the message is emitted (e.g. processReceived) as otherwise, the error will not get send to the server if no handler for the given target is registered. With version 0.8.0, test missing handler with return value should report an error does fail

3. Perform the resultProviderRegistry.contains check when on method is called as otherwise, calling on method with the same target multiple times in sequence will not throw because the onSubscription callback is not immediately called. With version 0.8.0 test registering multiple handlers for same target should raise an exception fails because of this

4. Always include the caught exception when rethrowing RuntimeException. There are four cases where the caught exception is not included in RuntimeException

All the above points are addressed with the latest commit in this PR.

I was looking at them and I am unsure of how much they are copied/inspired by official java library. Are they?

The ported tests do test the same use cases as tests in the official library, but were rewritten to use Kotlin Flows instead of RxJava library. Let me give you some examples:

Example 1 (on method):

Official library test:
https://github.com/dotnet/aspnetcore/blob/e8eeea60605f323014ab86ea34c2bd87ca8e84d7/src/SignalR/clients/java/signalr/test/src/main/java/com/microsoft/signalr/HubConnectionTest.java#L486-L508

Ported test:

SignalRKore/signalrkore/src/commonTest/kotlin/eu/lepicekmichal/signalrkore/OnTest.kt

Lines 12 to 30 in 6c81385

	fun `multiple handlers with one parameter should all be triggered`() = runTest {
	val completable = Completable()
	val value = Single<Int>()
	val add: suspend (Int) -> Unit = {
	value.updateResult { value -> (value ?: 0) + it }
	if (value.result == 20) {
	completable.complete()
	}
	}
	hubConnection.on("add", add)
	hubConnection.on("add", add)
	hubConnection.start()
	transport.receiveMessage("{\"type\":1,\"target\":\"add\",\"arguments\":[10]}$RECORD_SEPARATOR")
	completable.waitForCompletion()
	assertEquals(20, value.result)
	}

Example 2 (onWithResult method):

Official library test :
https://github.com/dotnet/aspnetcore/blob/main/src/SignalR/clients/java/signalr/test/src/main/java/com/microsoft/signalr/HubConnection.ReturnResultTest.java#L25-L43

Ported test:

SignalRKore/signalrkore/src/commonTest/kotlin/eu/lepicekmichal/signalrkore/OnWithResultTest.kt

Lines 12 to 28 in 6c81385

	fun `handler with no parameters should return a value`() = runTest {
	var called = false
	hubConnection.on("process", resultType = Boolean::class) {
	called = true
	true
	}
	hubConnection.start()
	val sentMessage = transport.nextSentMessage
	transport.receiveMessage("{\"type\":1,\"invocationId\":\"1\",\"target\":\"process\",\"arguments\":[]}$RECORD_SEPARATOR")
	val response = sentMessage.waitForResult()
	val expected = "{\"type\":3,\"invocationId\":\"1\",\"result\":true}$RECORD_SEPARATOR"
	assertEquals(expected, response)
	assertEquals(true, called)
	}

I am trying to avoid using their code as much as possible, therefore reluctant to accept the PR.

It is not possible to use their code directly as it is written in Java and they are using RxJava, junit and java.util.concurrent packages, which are not available in KMP. I don't see anything wrong having tests that are testing the same use cases as the official library does. Without them, it is not possible to say whether this library behaves the same as the official one.

[lepicekmichal]

Thanks, I like your changes this time around.

1. I can't seem to agree. I want the consistency of callback to always be with a result. However, your points are very much valid.
   • Exceptions being handled without .catch {}: It doesn't feel right to think that way; to use one override in a different use case just because its handling is easier for you. But I definitely could make that part of the error handling currently on the to-do list.
   • launching and stopping coroutine: That was not the case before 0.8.0 though. Right now yes, and I am going to think how to make it for the new way of on listeners, but this is not the way.
2. Looks good
3. Awesome
4. I missed those two, thank you!

As for tests.
I think I have to rewrite them a bit to my liking. These seem very copy-paste and very "java". I'll do it these few days.

As for your PR
If you would like to publish those changes, you could make separate PR for points 2,3, and 4 with no tests.
I would gladly merge those immediately and immediately publish a new version

[maca88]

I want the consistency of callback to always be with a result

That is fine for me, but if I would have the ability to choose between the following approaches:

var job = launch {
  hubConnection.on("printMessage", paramType1 = String::class).collect { (message) ->
    try {
      println(message)
    } catch (ex: Exception) {
      logger.log(Logger.Severity.ERROR, "Non-blocking invocation 'printMessage' method has thrown an exception", ex)
    }
  }
}

and

hubConnection.on("printMessage") { message: String -> println(message) }

I would choose the second approach every time :)

I think I have to rewrite them a bit to my liking. These seem very copy-paste and very "java". I'll do it these few days.

Fair point. I am looking forward to see how the rewritten tests will look like :)

If you would like to publish those changes, you could make separate PR for points 2,3, and 4 with no tests.

I created a separate PR for the mentioned points (#3)

[maca88]

Out of curiosity, I did check how the official C# client implemented the On method, and the approach it uses is quite interesting. The client exposes two low level methods, one for a callback with result and one without the result. Both methods take a list of parameter types, so that it is possible to create extension methods for how many parameters you need. The official library added extension methods up to 8 parameters, but it is possible to create extension methods for more parameters if wanted thanks to the exposed low level On methods.
In C#, using the same method name (On) for callbacks with results and no results works fine because the C# compiler is smarter. For some reason Kotlin compiler uses the Unit return type overload over the generic one (e.g. hubConnection.on("withReturn") { true } will prefer Unit instead of the generic RESULT: Any return type).
I pushed a proof of concept (using on2 name) to see how it would look like if such approach would be implemented in Kotlin. I had to use onWithResult for callbacks with result in order to avoid ambiguity.