Line 153 of WacLdp.ts - i.e. swallowed exception?

[michielbdejong]

Thanks to @pmcb55 for bringing up https://github.com/inrupt/wac-ldp/blob/master/src/lib/core/WacLdp.ts#L153.

Let's discuss how we can improve this! Any suggestions? Are you familiar with the debug package?

I had a look, and that error is logged, not swallowed. but let's see how we can improve it.

Because at that point, responding to the user failed, so reporting the error to the user is no longer an option. Can you think of anything the server could do at that point apart from logging it? The second argument should ensure that the log message include its details of message and stack trace, I think.

[pmcb55]

The point here is that this code continues on as if everything was perfectly fine, even if in fact it failed to return any response to the user at all (i.e. it is swallowing the exception).

I would argue that 'returning a response to the user' is a fundamentally important aspect of the Solid Server's job (it's kinda the whole point of it's existence), so failing to respond to the user (for whatever reason), is (potentially) a really big deal (it's probably 'just' a broken connection), but the point is the client made a request, we did something on their behalf, and they haven't gotten any response for that action we took.

Simply logging such a fundamental failure using a debug utility (i.e. debug is 'A tiny JavaScript debugging utility') is not sufficient, and instead this handle() method should be wrapping and re-throwing this exception to higher-level code that can decide what to do (e.g. report it to a central monitoring system, or write a message in the user's Pod (i.e. 'We failed to send a response to your message at <Time/Date>'), or whatever).

Or in other words, is it really appropriate for anything calling this 'handle()' code to only ever assume that the user must, always have gotten a response (or is that calling code supposed to tap into the debug stream somehow to determine if a debug message occured?).

[michielbdejong]

The calling code doesn't care much :) it's here: https://github.com/inrupt/pod-server/blob/master/src/server.ts#L188

You're right that it's confusing that WacLdp#handle returns a Promise<void> (could add a code comment there!) which the koa app awaits, but the way we're using koa is as a dispatcher if you will, not as a waterfall where another handler would retry.

So it's totally appropriate, and by design, that we don't bother the koa dispatcher with the task of logging errors there, but instead pass it to whichever logging system the higher-level code has configured.

Simply logging such a fundamental failure using a debug utility (i.e. debug is 'A tiny JavaScript debugging utility') is not sufficient

That's not true Pat, although I'll forgive you for thinking that, since you're looking at this with Java eyes. :)

First of all, your use of the term 'fundamental failure' for the case where a user closes the connection feels a bit dramatic, don't you think? What is your intention with the use of that term there, am I missing something which we haven't properly surfaced there yet?

Second, whether debug is the right tool to use for logging is a good, but separate question, that's not the discussion here (it has so far served our needs well and afaik it's a very popular choice in the NodeJS ecosystem, so I would think twice about moving away from it, but you're welcome to open a separate issue about that if you have any suggestions or requests there!).

And third, in fact, depending on which environment variables you set for the NodeJS process, debug logs to stdout, which then gets picked up by whatever orchestration tools have spun up the NodeJs process and hooked it into the higher-level systems on the server platform. You can learn more about such higher-level logging systems and how a node application like this would be deployed in production, for instance here: https://devcenter.heroku.com/articles/logging.

Does that answer your question?

PS: Action point taken to add a code comment on that : Promise<void> return value there, thanks!

[RubenVerborgh]

First of all, your use of the term 'fundamental failure' for the case where a user closes the connection feels a bit dramatic, don't you think?

I just want to point out that this will not even trigger in that case. Look at the code:

    try {
      debug('response is', response)
      return sendHttpResponse(response, {
        updatesVia: addBearerToken(this.updatesViaUrl, bearerToken),
        storageOrigin,
        idpHost: this.idpHost,
        originToAllow: requestOrigin || '*'
      }, httpRes)
    } catch (error) {
      debug('errored while responding', error)
    }
  }

This is synchronous code. How can the catch block be triggered?

• debug throws (not gonna happen)
• addBearerToken throws (hopefully not gonna happen)
• sendHttpResponse throws synchronously (very likely not going to happen)

Let me expand on that last case. The only reason sendHttpResponse would throw synchronously is when it is passed invalid arguments. Failures in connection, response generation, etc. etc., will not be caught because they are asynchronous.

So errored while responding, and hence most of the discussion above, is incorrect: it is errored while preparing the parameters to start sending a response.

[pmcb55]

So @RubenVerborgh's point seems to validate my earlier assertion for 100% branch coverage (i.e. there obviously wasn't ever a test that exercised this catch block!).

But @michielbdejong, I honestly don't know how you interpreted what I said above as: 'First of all, your use of the term 'fundamental failure' for the case where a user closes the connection feels a bit dramatic, don't you think?'...? I never said that at all. I said '(it's probably 'just' a broken connection)' (which I put in brackets to emphasize it as a side-comment, I put 'just' in quotes to emphasize not dismissing a closed connection in the middle of a request/response interaction as a minor thing that can just be dismissed), and then literally just before that sentence I said '...failing to respond to the user (for whatever reason)'.

So let me try again: I'm trying to say that failing to respond to a user request could be interpreted generally as a 'fundamental failure' within any server. That failure would certainly be explainable in the case of a client closing the connection before the response could be sent, but I was assuming this code could fail for many other reasons too (which was why I said 'for whatever reason' above). And even in the case of the client closing the connection, I still think giving the calling code a chance to do something about that would be really, really important here (i.e. if this code could throw exceptions, which it appears now it can't).

I'm not sure why you brought up debugging in the 2nd point. I've no issue with debug (I use it myself in the LIT). My point was simply logging an error does not somehow negate swallowing an exception (i.e. I assume you'd agree this is swallowing the exception if the debug() statement is removed completely, right? And what if debug() is configured to be switched off for this file or module, is the exception being swallowed then?).

And your 3rd point is about logging too, which seems to really emphaize the difference of opinion between us, in that you seem to think logging an exception here is effective error management for failing to respond to a user request, whereas I think such a failure is an important architectural flow control situation - i.e. it should wrap and re-throw this exception and very explicitly break the 'happy path' flow.

So no (even without @RubenVerborgh's comments above), your response doesn't answer my question at all. Is my explanation above any clearer now (i.e. ultimately my point was around flow-control)?