That's the problem with comments: the compiler won't tell you if it's wrong. The code probably started with sleeping for 90 seconds, and then they changed it to 30s and the compiler was still happy.
I had a coworker referencing another line by its line number in a comment. When I did code review the line number wasn't right because he made other changes after writing that comment. Safe to say it didn't pass code review.
There's a simple rule, comment the WHY not the WHAT. Of course there's an exception to every rule so you might end up doing some magic for optimization (fast inverse square root comes to mind) so you'd better comment the WHAT in that case instead of `what the fuck?`
the comment is silly anyway.
time.sleep(30) does not need a comment "sleep for 30 seconds".
Comment why, not what happens "here we need a sleep because the system is not that fast". Yes that can get outdated too, but at least is more informative and not tied to actual values.
So that later, when the customer brings up a meaningless, senseless complaint (or general, vague sense of displeasure), you can bring that sleep value down, and voilá - you just did some “hard-core performance tuning,” the app runs faster, and the customer is happy with you now.
The printing is redundant with the logging, indicating the author didn't really know what they were doing. I'm inclined to defer to OP's judgement on this one
I wish I had any reason to think this was the goal of these lines.
Also, I don‘t think just catching any and all Exceptions and continuing the program is a good idea
I’m telling you that’s a really common thing to do when you don’t want to expose internal error details (which can’t expose stacktraces, hostnames, etc) to your end users.
Basically every piece of production software you’ve ever used does some variation of this.
Now, maybe that’s not what’s happening here. Maybe this code is entirely shit and needlessly swallows errors that could otherwise be useful upstream. But we can’t see that.
> I wish I had any reason to think this was the goal of these lines.
This is bog standard, this is the default for something that returns right to user space.
Why do you need "any reason" to consider mundane, standard, structure anything other than mundane standard structure?
If that string goes straight to the UI in front of the user, it's fine to do a catch-all. You don't want to leak anything to the users. Say, there is an inner exception: "...another exception occurred: APITokenInvalidException: API token MySecretApiToken123456 is not valid for www.myserviceprovider.com/api/my-company-id/my-business-unit/personally-identifiable-information" and now the user knows your secrets and has access to the worst leak. Better to just say "Error" instead.
That being said. It does not look like what the intention was :P
Yeah if that were the case I‘d kinda understand this (still don‘t get the double output, but maybe logging acts weird). But this does really have a frontend that is accessible by people who should not see what goes on behind the scenes. We work extremely closely with that customer, they might even have direct access to this code.
> Come on, it prints the exception at the first line of the catch block and then returns simply a string “Error”
That..... sounds like a pretty standard try/catch wrapper. Usually it's abstracted by a couple layers, but it's still fundamentally *exactly the same*.
Sure, the error message could be nicer, but there is absolutely nothing wrong with this. If you don't catch globally unhandled errors and return something benign to user space, you are definitely doing things wrong.
______________________________________
> it prints the exception at the first line of the catch block
It clearly prints it (Which will print the message), and then logs it. Do you prefer to not log & monitor your error rates?
> and then returns simply a string “Error”
This is the only part that could make it bad, and only because of the message, which is communication/product not code.
Catching the generic exception makes sense to log any unexpected errors, and if you don't want the program to crash because of a bad request (e.g. server, or message queue processing)
I hard disagree. You should catch anything you expect to happen, handle it, and move on. Anything else is a bug.
Only place where it makes sense is at the very top level of your code, which, if abstracted enough, is likely a very small portion of your codebase.
Shouldn’t your orchestrator take care of restarting your instance? You have no idea if the state is even viable after an unexpected exception, it might just spiral. IMO better to shutdown and spin a new instance, it’s why you run more than one.
As I wrote in my response to the other commenter, that really depends on why the exception happend, like what broke, was it transient? did it corrupt internal state? how bad is that corruption? etc.
We have observed some exceptions that would bust some part of the service so much so that every request after the initial corruption would generate that exception, so restarts are a better idea in that case, since it then always works after. And restarting some pods or containers is often quick and painless at least for our architecture, most services will be serving requests within 10 seconds. And k8s is perfectly capable of handling the routing of requests when you have your probes setup correctly.
And then later we can try and track down the exception and add some specific exception handling to ACTUALLY handle and repair the cause of the exception. Instead of just hoping for the best.
And why would you want to do it if literally catching it is cheaper, more reliable, simpler and you get more info that way? Why would you want to add work (and reduce avaiability) if it gives you nothing in return?
But the thing is IS it more reliable? The whole point of an unexpected exception is that it is in fact unexpected, some state might be corrupted in the service and it for sure is not going to heal itself. So a quick restart is just the easiest way to mitigate an unforeseen consequences of the exception. And especially in kubernetes or something a restart is fully baked into the platform and shouldn't even take that long if your services are sane.
I'm not saying you shouldn't log these or investigate or whatever, but that is more an after the fact thing so that next time the exception is not unexpected. Maybe it turns out that bit of database connection handling and pooling needs a bit of auto healing it didn't have or what have you.
> Only place where it makes sense is at the very top level of your code
Ah yes, the, "let everything fail spectacularly" way of developing software...
If you have an important path and you don't know what might be thrown anywhere down the stack you put a generic exception there and *gracefully* handle it and log it. You can then go back and add more specific handling or precondition assertions.
Who TF writes production code that doesn't have graceful, unexpected, exception handling...?
That is why I said "the top level of your code".
When you don't know what is thrown, it can cause bugs and make future issues super hard to diagnose. What you're saying is essentially "I don't care what the problem is, keep running the program." And is not good software design.
Again, the only place it makes sense is at the top level of your code, specifically within a custom top level error handler, but even THEN it is still better to handle specific exceptions, and THEN FINALLY use a general exception as the final fallback.
I think even in the top level it doesn't make much sense. I mean if exception raises up to main(), what could you do with that? In 95% cases you can't restore execution and the only thing you can do is to log error and let program fail. There's ways to make it without catching generic exception in main().
That... really depends on what your application is doing. If you're writing a server and a client makes a bad request, you shouldn't crash the entire system, you should just move on to the next request.
> Then you must process bad request error closer to the place, where it's raised
Literally the point of a try/catch with a generic Exception when you don't know what may throw and you *need* it to be handled gracefully, we've now gone full circle.
> The only thing really worth changing is not capturing a generic Exception.
You prefer to not catch a generic exception and instead not handle unexpected exceptions...?
Wut.
standard, and broadly awful.
It's one thing if there is real concern about the error revealing internals, but you can at least give some feedback when it's going to be helpful to them.
This.... is normal when returning an error to the user.
Often it's an abstraction, but if it isn't, it looks like this.
You log it, and return an unspecified error if it's unexpected, or a specific one if not.
It's also common to wrap your entire request pipeline in a try/catch, so if an unhanded exception occurs, you can return a generic error and log it for alerting or other monitoring.
>A common practice is not necessarily a good practice.
Sure you can put your head in the sand instead of trying to understand what common patterns are as well I guess.
Common solutions to common problems exist because they work and work well. Not because they're bad.
It's a **realistic** coding philosophy if you work in a massive, disorganized company on a distributed architecture, with contractors and junior engineers who are liable to push code out that does God-knows-what.
*Image Transcription: Code*
---
except Exception as e:
print(e)
logging.error(e)
return "Error"
---
^(This post was transcribed unofficially; it dropped off the queue before I had a chance to transcribe it, so I'm doing so in an unofficial capacity. If you're interested in why I've done this,) [^(see here for more details)](https://old.reddit.com/user/MurdoMaclachlan/comments/upg4zp/faq_transcribing/)^(.)
I'm not against it but I would personally do a \*little\* parsing. Like did a file not exist? Was an input on the form incorrect?
I agree the stack trace should not be shown to the end user, but I would hope it would make it more informative than just "Error"
Eh.
Depends on what level.
For example, an API consumer probably would be better to have some more details returned. But for something like a UI which a non-developer is using? Just displaying a message like "Oh no! Something went wrong" is perfectly fine.
> Just displaying a message like "Oh no! Something went wrong" is perfectly fine.
disagree.
It's like the worst situation.
Paying for food on Uber, just saying "transaction failed" again and again, then you call and they say immediately "it's because your card is not a local currency card".
How about you actually TELL me that when it fails! If you know why it failed, let me know, so I can do something different!
It's not always the case. For example, you have an API that should not be used by the public, but must be in the public internet for , for example for the mobile client app to exchange data with it. You don't want to return "Error while proceeding fund withdrawal: Missing this_key and that_key". You'd rather want to hide those details.
If it's a catch-all exception like in this case, there's possibly nothing you can do at that point and it's more or less undefined behaviour and there just not to leak anything specific or to give a nice "Error" instead of a stack trace the user has no idea what to do with.
If it's an error like that which is resolvable by the user then I agree.
But if it's an internal error because something randomly failed then they don't need the specifics. A generic error is fine, but I should have qualified that with "in some circumstances."
If I'm uploading a file and it fails because it's too big then yes I'd like to be told that.
Later, there's also the fun lines ``` # sleep for 90 seconds time.sleep(30) ``` with no further explanation
People: You need to comment your code Comments in code:
That's the problem with comments: the compiler won't tell you if it's wrong. The code probably started with sleeping for 90 seconds, and then they changed it to 30s and the compiler was still happy. I had a coworker referencing another line by its line number in a comment. When I did code review the line number wasn't right because he made other changes after writing that comment. Safe to say it didn't pass code review.
True. Comments are easily outdated or straight out lie what is going on, because someone doesn't understand what the method does or why
There's a simple rule, comment the WHY not the WHAT. Of course there's an exception to every rule so you might end up doing some magic for optimization (fast inverse square root comes to mind) so you'd better comment the WHAT in that case instead of `what the fuck?`
Bigger problem is using a magic number. Just define it and you only have to change the one variable value in the future.
the comment is silly anyway. time.sleep(30) does not need a comment "sleep for 30 seconds". Comment why, not what happens "here we need a sleep because the system is not that fast". Yes that can get outdated too, but at least is more informative and not tied to actual values.
I strongly agree. I have no idea why that code needed to sleep for so long
> time.sleep(30) does not need a comment "sleep for 30 seconds". Well I guess it does if you write it with Copilot.
Lol, I've been using Copilot and it is pretty fun, but my mind was blown by my coworker who said "it writes such good comments!"
So that later, when the customer brings up a meaningless, senseless complaint (or general, vague sense of displeasure), you can bring that sleep value down, and voilá - you just did some “hard-core performance tuning,” the app runs faster, and the customer is happy with you now.
It started at 90 seconds so they could improve performance by 66% easily.
Well, clearly that is polyglot, not a comment. It sleeps for a different amount of time depending on which language you run it as. /s
TIL polyglot
That's because one hour on this planet is seven years on earth.
Sleeps 30% of the time, every time.
Returning a generic error message to the end user while logging all the details for internal debugging is pretty standard stuff.
The printing is redundant with the logging, indicating the author didn't really know what they were doing. I'm inclined to defer to OP's judgement on this one
I wish I had any reason to think this was the goal of these lines. Also, I don‘t think just catching any and all Exceptions and continuing the program is a good idea
I’m telling you that’s a really common thing to do when you don’t want to expose internal error details (which can’t expose stacktraces, hostnames, etc) to your end users. Basically every piece of production software you’ve ever used does some variation of this. Now, maybe that’s not what’s happening here. Maybe this code is entirely shit and needlessly swallows errors that could otherwise be useful upstream. But we can’t see that.
> I wish I had any reason to think this was the goal of these lines. This is bog standard, this is the default for something that returns right to user space. Why do you need "any reason" to consider mundane, standard, structure anything other than mundane standard structure?
If that string goes straight to the UI in front of the user, it's fine to do a catch-all. You don't want to leak anything to the users. Say, there is an inner exception: "...another exception occurred: APITokenInvalidException: API token MySecretApiToken123456 is not valid for www.myserviceprovider.com/api/my-company-id/my-business-unit/personally-identifiable-information" and now the user knows your secrets and has access to the worst leak. Better to just say "Error" instead. That being said. It does not look like what the intention was :P
Yeah if that were the case I‘d kinda understand this (still don‘t get the double output, but maybe logging acts weird). But this does really have a frontend that is accessible by people who should not see what goes on behind the scenes. We work extremely closely with that customer, they might even have direct access to this code.
Come on, it prints the exception at the first line of the catch block and then returns simply a string “Error”. By all means this is a bad code. :)
> Come on, it prints the exception at the first line of the catch block and then returns simply a string “Error” That..... sounds like a pretty standard try/catch wrapper. Usually it's abstracted by a couple layers, but it's still fundamentally *exactly the same*. Sure, the error message could be nicer, but there is absolutely nothing wrong with this. If you don't catch globally unhandled errors and return something benign to user space, you are definitely doing things wrong. ______________________________________ > it prints the exception at the first line of the catch block It clearly prints it (Which will print the message), and then logs it. Do you prefer to not log & monitor your error rates? > and then returns simply a string “Error” This is the only part that could make it bad, and only because of the message, which is communication/product not code.
Yeah but they're logging it twice
once to stdout and once to a persistent log. it's maybe overkill, but not worthy of this sub IMO.
Agreeed. The only thing really worth changing is not capturing a generic Exception.
Catching the generic exception makes sense to log any unexpected errors, and if you don't want the program to crash because of a bad request (e.g. server, or message queue processing)
I hard disagree. You should catch anything you expect to happen, handle it, and move on. Anything else is a bug. Only place where it makes sense is at the very top level of your code, which, if abstracted enough, is likely a very small portion of your codebase.
Bug should not crash prod. Log it and move on Generic catch all is extremely common esp front facing
Shouldn’t your orchestrator take care of restarting your instance? You have no idea if the state is even viable after an unexpected exception, it might just spiral. IMO better to shutdown and spin a new instance, it’s why you run more than one.
isn’t restarting the instance a terrible idea for a simple unexpected exception?
As I wrote in my response to the other commenter, that really depends on why the exception happend, like what broke, was it transient? did it corrupt internal state? how bad is that corruption? etc. We have observed some exceptions that would bust some part of the service so much so that every request after the initial corruption would generate that exception, so restarts are a better idea in that case, since it then always works after. And restarting some pods or containers is often quick and painless at least for our architecture, most services will be serving requests within 10 seconds. And k8s is perfectly capable of handling the routing of requests when you have your probes setup correctly. And then later we can try and track down the exception and add some specific exception handling to ACTUALLY handle and repair the cause of the exception. Instead of just hoping for the best.
And why would you want to do it if literally catching it is cheaper, more reliable, simpler and you get more info that way? Why would you want to add work (and reduce avaiability) if it gives you nothing in return?
But the thing is IS it more reliable? The whole point of an unexpected exception is that it is in fact unexpected, some state might be corrupted in the service and it for sure is not going to heal itself. So a quick restart is just the easiest way to mitigate an unforeseen consequences of the exception. And especially in kubernetes or something a restart is fully baked into the platform and shouldn't even take that long if your services are sane. I'm not saying you shouldn't log these or investigate or whatever, but that is more an after the fact thing so that next time the exception is not unexpected. Maybe it turns out that bit of database connection handling and pooling needs a bit of auto healing it didn't have or what have you.
> Only place where it makes sense is at the very top level of your code Ah yes, the, "let everything fail spectacularly" way of developing software... If you have an important path and you don't know what might be thrown anywhere down the stack you put a generic exception there and *gracefully* handle it and log it. You can then go back and add more specific handling or precondition assertions. Who TF writes production code that doesn't have graceful, unexpected, exception handling...?
That is why I said "the top level of your code". When you don't know what is thrown, it can cause bugs and make future issues super hard to diagnose. What you're saying is essentially "I don't care what the problem is, keep running the program." And is not good software design. Again, the only place it makes sense is at the top level of your code, specifically within a custom top level error handler, but even THEN it is still better to handle specific exceptions, and THEN FINALLY use a general exception as the final fallback.
I think even in the top level it doesn't make much sense. I mean if exception raises up to main(), what could you do with that? In 95% cases you can't restore execution and the only thing you can do is to log error and let program fail. There's ways to make it without catching generic exception in main().
That... really depends on what your application is doing. If you're writing a server and a client makes a bad request, you shouldn't crash the entire system, you should just move on to the next request.
Then you must process bad request error closer to the place, where it's raised. Do not let it raise to the top level.
> Then you must process bad request error closer to the place, where it's raised Literally the point of a try/catch with a generic Exception when you don't know what may throw and you *need* it to be handled gracefully, we've now gone full circle.
> The only thing really worth changing is not capturing a generic Exception. You prefer to not catch a generic exception and instead not handle unexpected exceptions...? Wut.
With propper logging set up there is no need to duplicate the call. That's bad in any case.
standard, and broadly awful. It's one thing if there is real concern about the error revealing internals, but you can at least give some feedback when it's going to be helpful to them.
This.... is normal when returning an error to the user. Often it's an abstraction, but if it isn't, it looks like this. You log it, and return an unspecified error if it's unexpected, or a specific one if not. It's also common to wrap your entire request pipeline in a try/catch, so if an unhanded exception occurs, you can return a generic error and log it for alerting or other monitoring.
A common practice is not necessarily a good practice.
>A common practice is not necessarily a good practice. Sure you can put your head in the sand instead of trying to understand what common patterns are as well I guess. Common solutions to common problems exist because they work and work well. Not because they're bad.
It works because you’ve accepted undefined behavior as a core of your program. That is a poor coding philosophy.
It's a **realistic** coding philosophy if you work in a massive, disorganized company on a distributed architecture, with contractors and junior engineers who are liable to push code out that does God-knows-what.
*Image Transcription: Code* --- except Exception as e: print(e) logging.error(e) return "Error" --- ^(This post was transcribed unofficially; it dropped off the queue before I had a chance to transcribe it, so I'm doing so in an unofficial capacity. If you're interested in why I've done this,) [^(see here for more details)](https://old.reddit.com/user/MurdoMaclachlan/comments/upg4zp/faq_transcribing/)^(.)
I love it when the detail is gobbled up and replaced by “error” My absolute fav
This is pretty typical stuff if that return value is shown to a client and you don't want to leak the internal details of the error to them
I should have checked what happens with "Error". I bet it‘s not even checked for.
My guess - that is what is shown to the end user lol.
Well... Good? You shouldn't show your end user the full stack trace of an error, that's a security problem. Not the problem here.
I'm not against it but I would personally do a \*little\* parsing. Like did a file not exist? Was an input on the form incorrect? I agree the stack trace should not be shown to the end user, but I would hope it would make it more informative than just "Error"
You should return some kind of helpful information though.
Eh. Depends on what level. For example, an API consumer probably would be better to have some more details returned. But for something like a UI which a non-developer is using? Just displaying a message like "Oh no! Something went wrong" is perfectly fine.
> Just displaying a message like "Oh no! Something went wrong" is perfectly fine. disagree. It's like the worst situation. Paying for food on Uber, just saying "transaction failed" again and again, then you call and they say immediately "it's because your card is not a local currency card". How about you actually TELL me that when it fails! If you know why it failed, let me know, so I can do something different!
It's not always the case. For example, you have an API that should not be used by the public, but must be in the public internet for, for example for the mobile client app to exchange data with it. You don't want to return "Error while proceeding fund withdrawal: Missing this_key and that_key". You'd rather want to hide those details.
If it's a catch-all exception like in this case, there's possibly nothing you can do at that point and it's more or less undefined behaviour and there just not to leak anything specific or to give a nice "Error" instead of a stack trace the user has no idea what to do with.
> You don't want to return "Error while proceeding fund withdrawal: Missing this_key and that_key" you can still do "bad request"
If it's an error like that which is resolvable by the user then I agree. But if it's an internal error because something randomly failed then they don't need the specifics. A generic error is fine, but I should have qualified that with "in some circumstances." If I'm uploading a file and it fails because it's too big then yes I'd like to be told that.
An oopsie woopsie has occured!
at least it gets logged! could be much worse
The number of people thinking that any exception you forgot to catch should result in vulnerability in your application is too damn high.
Let me improve this masterpiece ```python ... except Exception as e: print(e) print(traceback.print_exc()) logging.error(e) logging.error(traceback.print_exc()) print("Error in", __name__) return "Error" ```
Happy cake day :)
`# check if x is greater than 3` `if (x > 3)`
It’s more common than you would think in production code sadly
If you return a string in an exception handler i will kill your child process
This looks fine to me?