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

.mapConnect() and .doOnRequestError() are not called when .headersWhen() is used #2904

Closed
mateuszrzeszutek opened this issue Sep 20, 2023 · 4 comments · Fixed by #2912
Closed
Assignees
Labels
type/bug A general bug
Milestone

Comments

@mateuszrzeszutek
Copy link

mateuszrzeszutek commented Sep 20, 2023

Expected Behavior

I would expect that the HTTP client executes its callbacks regardless of whether the headers were set using .headersWhen() or headers(). Currently this is not the case, .mapConnect() and .doOnRequestError() callbacks are not executed when deferred config is used.

Actual Behavior

.mapConnect() and .doOnRequestError() callbacks are not executed when deferred config is used.

Steps to Reproduce

This test prints out the two messages to stdout:

  @Test
  void headers() {
    HttpClient.create()
        .headers(h -> h.set("test", "test"))
        .mapConnect(connection -> {
          System.out.println("mapConnect()");
          return connection;
        })
        .doOnRequestError((request, throwable) -> System.out.println("doOnRequestError()"))
        .get()
        .uri("http://192.0.2.1")
        .response()
        .block();
  }

This one does not:

  @Test
  void headersWhen() {
    HttpClient.create()
        .headersWhen(h -> Mono.just(h.set("test", "test")))
        .mapConnect(connection -> {
          System.out.println("mapConnect()");
          return connection;
        })
        .doOnRequestError((request, throwable) -> System.out.println("doOnRequestError()"))
        .get()
        .uri("http://192.0.2.1")
        .response()
        .block();
  }

Possible Solution

Mono<? extends Connection> mono;
if (config.deferredConf != null) {
return config.deferredConf.apply(Mono.just(config))
.flatMap(MonoHttpConnect::new);
}
else {
mono = new MonoHttpConnect(config);
}

Removing the return in line 113 would allow the code in lines 120-133

if (config.doOnConnect() != null) {
mono = mono.doOnSubscribe(s -> config.doOnConnect().accept(config));
}
if (config.doOnRequestError != null) {
mono = mono.onErrorResume(error ->
Mono.deferContextual(Mono::just)
.doOnNext(ctx -> config.doOnRequestError.accept(new FailedHttpClientRequest(ctx, config), error))
.then(Mono.error(error)));
}
if (config.connector != null) {
mono = config.connector.apply(mono);
}
to happen before the Mono is returned.

Your Environment

  • Reactor version(s) used: this happens on any version starting from 1.0
  • Other relevant libraries versions (eg. netty, ...):
  • JVM version (java -version): OpenJDK Runtime Environment Temurin-17.0.2+8 (build 17.0.2+8)
  • OS and version (eg. uname -a): Darwin Kernel Version 22.6.0
@violetagg
Copy link
Member

@mateuszrzeszutek Would you like to create a PR?

@violetagg violetagg removed the status/need-triage A new issue that still need to be evaluated as a whole label Sep 20, 2023
@mateuszrzeszutek
Copy link
Author

Sure 👍
I'll submit a fix tomorrow

@violetagg
Copy link
Member

Please use as a base 1.0.x branch, later we will forward merge to main branch.

@violetagg violetagg added this to the 1.0.37 milestone Sep 20, 2023
@mateuszrzeszutek
Copy link
Author

Hey @violetagg ,
I'll need to make sure with my employer that I can sign the CLA -- it might take me a while, so unfortunately don't count on a quick fix from me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
2 participants