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

Errors during wsBeforeUpgrade do not invoke the Request Logger #2218

Open
LoonyRules opened this issue Apr 7, 2024 · 1 comment
Open

Errors during wsBeforeUpgrade do not invoke the Request Logger #2218

LoonyRules opened this issue Apr 7, 2024 · 1 comment
Labels

Comments

@LoonyRules
Copy link
Contributor

LoonyRules commented Apr 7, 2024

Actual behavior (the bug)
An error occurring inside of a wsBeforeUpgrade(...) handler, eg: authenticating requests before upgrading to a websocket resulting in an UnauthorizedResponse being thrown, results in no RequestLogger method being invoked.

Expected behavior
As the HTTP Request has not be upgraded to a WebSocket yet, I would expect the RequestLogger.handle(@NotNull Context ctx, @NotNull Float executionTimeMs) throws Exception method to be invoked.

I would not expect the WebSocket Request Logger to be executed, as the HTTP Request is not an Upgraded Request yet.

To Reproduce

  1. Register a Request Logger for HTTP requests:
ctx.requestLogger.http((ctx, executionTimeMs) -> {
  LOGGER.info("{} {} {} {} {}ms", ctx.ip(), ctx.method(), ctx.path(), ctx.status(), executionTimeMs);
});
  1. Register a Request Logger for WebSockets:
 config.requestLogger.ws(wsConfig -> {
  wsConfig.onConnect(ctx -> {
    Context upgradeCtx = ctx.getUpgradeCtx$javalin();
    LOGGER.info("{} {} {} {}", upgradeCtx.ip(), upgradeCtx.method(),upgradeCtx.path(), upgradeCtx.status());
  });
});
  1. Register your WebSocket endpoints and wsBeforeUpgrade handler:
javalin.wsBeforeUpgrade("/ws", ctx -> {
  throw new UnauthorizedResponse();
});
// Required otherwise the wsBeforeUpgrade doesn't match anything.
javalin.ws("/ctx", wsConfig -> {});
  1. Invoke a HTTP Upgrade Request to the /ws endpoint.
  2. Observe that you get a 401 Unauthorized response.
  3. Observe that nothing is no logged.

Additional context
You can hack around this by using Exception Handlers but if you have multiple exception handlers, or error types then this gets very messy and is not a great solution.

If you did this same pattern for HTTP requests:

javalin.beforeMatched("/http", ctx -> {
  throw new UnauthorizedResponse();
});
javalin.get("/http", ctx -> {});

then you get the following log output via the RequestLogger.handle(...) method:

[JettyServerThreadPool-Virtual-21] INFO com.example.WebApplication - 127.0.0.1 GET /http 401 Unauthorized 106.1861ms
@tipsy
Copy link
Member

tipsy commented Apr 8, 2024

Thanks @LoonyRules. I agree - added the BUG label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants