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

Exceptions on callbacks are always logged to stderr #518

Open
mateusduboli opened this issue Apr 19, 2023 · 7 comments
Open

Exceptions on callbacks are always logged to stderr #518

mateusduboli opened this issue Apr 19, 2023 · 7 comments

Comments

@mateusduboli
Copy link

mateusduboli commented Apr 19, 2023

Hi Folks!

HttpKit Version: v2.6.0

When there is an exception on the callback function from http/request the exception will always be printed using HttpUtil/printError.

This is undesirable as there is no way to control over this information and make it difficult to monitor for problems when there are expected exceptions.

A proposed solution, would be to use the ContextLogger from the client variable calculated just before the call.

But Ideally these logs should comply with slf4j or some other interface that would allow for more fine grained control using configuration files.

@mateusduboli mateusduboli changed the title Use client's ContextLogger when dealing with callback issues Exceptions on callbacks are always logged to stderr Apr 19, 2023
@ptaoussanis
Copy link
Member

@mateusduboli Hi Mateus!

Have you considered wrapping your callback function to catch + handle exceptions in your preferred way? If that's for some reason not possible or desirable, could you please explain why so that I can better understand your situation?

Thanks!

@kumarshantanu
Copy link
Collaborator

A proposed solution, would be to use the ContextLogger from the client variable calculated just before the call.

But Ideally these logs should comply with slf4j or some other interface that would allow for more fine grained control using configuration files.

ContextLogger is an interface, so you can log to anything in the implementation. This eliminates 3rd party dependency.

@mateusduboli
Copy link
Author

mateusduboli commented Apr 20, 2023 via email

@ptaoussanis
Copy link
Member

There are options to resolve it, like not using exceptions as a control
flow

Could you please expand on this? Where are exceptions being used as control flow?

however it still feel that the library does not provide enough
flexibility to for instance send these exceptions to a file.

By "the library", do you mean http-kit or Martian?

If http-kit: is there some other place (besides the client request callback) where you want to control the behaviour on errors and cannot?

@mateusduboli
Copy link
Author

mateusduboli commented Apr 24, 2023

Could you please expand on this? Where are exceptions being used as control flow?

This is being used in my application, where when the status code is above 400 it will wrap the response in an exception, this exception is used on my "handler" code to perform decisions.

(require '[org.httpkit.client :as http])

(defn process-response [{status :status :as response}]
  (if (>= status 400)
    (throw (ex-info "Error in response" {:response response}))
    response))
  
@(http/request {:method :get
                :url "http://httpstat.us/400"}
                process-response)

This code both returns the exception and prints it on stderr, in my case the process-response is wrapped by another function would be something like:

(defn safe-process-response [response]
  (try
    (process-response response)
    (catch Throwable t
      (log-and-suppress-error t))

The problem is, IF I don't have access to the code that manages these callbacks, I cannot suppress the exception going to the stderr, which I could do for other logs in httpkit by binding the org.httpkit.client/*default-client*.

By "the library", do you mean http-kit or Martian?

I mean http-kit, as these logs are managed by it.

If http-kit: is there some other place (besides the client request callback) where you want to control the behaviour on errors and cannot?

I think any place that uses a hardcoded reference to HttpUtils/printError is a candidate of this problem.

https://github.com/search?q=repo%3Ahttp-kit/http-kit%20printError&type=code

@ptaoussanis
Copy link
Member

I think any place that uses a hardcoded reference to HttpUtils/printError is a candidate of this problem.

From what I can see there, http-kit appears to only use printError in 2 cases where the user is in control of the function that could throw.

I.e. the design appears to be that these printErrors occur only in exceptional circumstances, and as a fallback when the user hasn't provided their own error handling.

That seems generally reasonable to me, given that custom error handling is fundamentally a superset of custom error logging.

Though I understand that in your case, the library you're using seems to be obscuring access to custom error handling.

Assuming there's no way of providing a custom handler in Martian, it seems the options are:

  1. Provide a PR for http-kit to allow custom error logging
  2. Advocate for an enhancement to Martian (if applicable) to allow custom error handling, since I suspect that might be useful in general (?)

The two aren't mutually exclusive, and both could be independently worthwhile.
Would be happy to take a look at a PR for (1.) if you like.

Cheers :-)

@mateusduboli
Copy link
Author

mateusduboli commented Apr 26, 2023

Studying the code I wanted to double check some understandings that I have about org.httpkit.client/request and its relation to HttpClient.

  • HttpClient runs as a single background thread that takes requests and enqueues then, doing a busy loop to find the next request to execute or to process it's response.
  • To get the response, still on java land, an implementation of the IRespListener interface is the one responsible to decode and process the communication.
  • That interface will send the parsed body, headers and status to a new instance of IRespHandler, that handler will be executed on a different ThreadPool, namely the worker-pool
  • The interaction between the java and the clojure is made through the implementation of IRespListener and IRespHandler.
  • For handling asynchronous requests a promise is used to hold the value, and the deliver call that wraps all the results for the reyfied IRespHandler.
  • The callback runs on the worker-pool thread-pool as it is done on the IRespHandler.

What I'm trying to organize is the relation between the callback and the HttpClient to be able to make use of the existing configured loggers.

However, because they are executed in different thread-pools and in contexts which the HttpClient is not a dependency it seems not to be possible.

From that, it seems the simplest solution would be to add a error-logger parameter to the request call, or to use a *error-logger* global, this will NOT resolve the problem of other instances of HttpUtils.printError, such as TimerService.

Do you see any other ideas?

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

No branches or pull requests

3 participants