Replace rxjava with reactor #541
Conversation
…o replace-rxjava-with-reactor # Conflicts: # components/client/src/main/java/com/hotels/styx/client/Connection.java # components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java # components/client/src/main/java/com/hotels/styx/client/connectionpool/ExpiringConnection.java # components/client/src/main/java/com/hotels/styx/client/connectionpool/stubs/StubConnectionFactory.java # components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/HttpRequestOperation.java # components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java # components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyToStyxResponsePropagator.java # components/client/src/test/unit/java/com/hotels/styx/client/StyxHostHttpClientTest.java # components/client/src/test/unit/java/com/hotels/styx/client/netty/connectionpool/NettyToStyxResponsePropagatorTest.java
Create an exception lazily only when necessary.
* Fixed Java unit tests so they run along with Kotlin tests in the components/proxy module * Removed tests containing System.exit(); as Surefire does not support tests calling System.exit().
There was a problem hiding this comment.
Nice work @OwenLindsell.
Only question though, and therefore requesting changes: What happened to pushesHttpContentOnChannelReadComplete test?
IIRC this is testing an important corner case in Netty/Styx interaction. If not correctly handled leads to hanging responses.
Otherwise everything looks good.
| */ | ||
| Observable<LiveHttpResponse> write(LiveHttpRequest request); | ||
| Flux<LiveHttpResponse> write(LiveHttpRequest request); |
There was a problem hiding this comment.
We could convert this to Mono. But that would be a separate increment.
There was a problem hiding this comment.
Do we need to have Flux (or Mono) in the signature here? I see an advantage of "Mono" as it depicts there's only going to be 0..1 events, but otherwise I would try to expose Reactive-streams types instead of the one from reactor-core.
| Mono.from(RxReactiveStreams.toPublisher( | ||
| connection.write(networkRequest) | ||
| .doOnTerminate(connection::close))) | ||
| Mono.from(connection.write(networkRequest) |
There was a problem hiding this comment.
For the record:
As discussed, Publisher<LiveHttpResponse> cancel event could occur whilst connection is being used in flatMap operator. In this case the connection needs to be closed.
Let's look into this as a separate improvement.
There was a problem hiding this comment.
Should we use doFinally() here instead of having to include doOnComplete, doOnError, doOnCancel ...?
| StepVerifier.create(response.body()) | ||
| .then(channel::runPendingTasks) // Execute onSubscribe in FSM | ||
| .then(() -> channel.pipeline().fireExceptionCaught(new RuntimeException())) // Will emmit BadHttpResponseException | ||
| .then(() -> channel.pipeline().fireChannelInactive()) // Will emmit TransportLostException |
...unit/java/com/hotels/styx/client/netty/connectionpool/NettyToStyxResponsePropagatorTest.java
Show resolved
Hide resolved
…o replace-rxjava-with-reactor
...unit/java/com/hotels/styx/client/netty/connectionpool/NettyToStyxResponsePropagatorTest.java
Show resolved
Hide resolved
No description provided.