change how error handling for sse works

[elee1766]

the panic handling sort of isn't great.

i changed it to use errors, and added a print so that the functionality isnt really changed (although really, the print shoudl probably be in the caller of Send?)

probably even more idiomatic would be to store the error in the struct, so that the caller could check if the connection has errored, and attempt to create a new one or not?

[delaneyj]

I'm on the fail it's an invariant,

https://crawshaw.io/blog/go-and-sqlite

Almost all programs making SQL queries define the text of those queries statically. (The only obvious exception is if you are writing an SQL REPL.) Doing otherwise is a security risk. It does not make sense to try and handle the error from an SQL typo at run time. So the standard way to prepare a statement, the Prep method, does not return an error. Instead it panics if the SQL fails to compile.

The behavior of the Prep method is spiritually similar to regexp.MustCompile, which is designed to be used with regular expression string literals. As a side effect this means slightly fewer lines of code are required to execute a query, but most importantly, it means the bug is treated correctly.

Point being is if this fails it's :100: a dev error and should be immediate. I'd be fine with a SendWithErr or something

[elee1766]

this is functionally equivalent to what already exists. I didn't remove the log on error, and it will log in the same place, so it's equally immediately.

the difference here is that the error is not being thrown away, so the code Send now at least has the option to handle the error. previously, this information was discarded. again, it "fails" (through printing) equally early as the code pre-change.

realistically, the print should be removed, and the log should be on catching the error from send.

It would make more sense to create MustSend which wraps Send, and perhaps prints the error, if you want to be able to keep that behavior. i'll change it to that now, since it's more idomatic go.

in reality, the errors should be handled, such that ignorable errors (say, connection closed) can be ignored, but other panics which may point to other issues are at the very least printed, and perhaps even re-thrown

regardless, emitting panics then catching them instead of returning errors where you can is awful code, in terms of style, convention, and performance, so it should be changed somehow.

[delaneyj]

Looking good, need to make sure to update all examples if you want to use your new way at the preferred default

[elee1766]

will find some time to update examples

is there a full test suite i can run to ensure everything is good? I've just been starting the example app with task -w hot

[delaneyj]

https://github.com/delaneyj/datastar/tree/main/packages/playwright

[elee1766]

i went around and i don't see any references to the Send method anywhere in the repo or here: https://github.com/delaneyj/realworld-datastar. if there are other examples i should update let me know.

i tried running the playwright tests, and the same tests fail when im running task -w hot on origin/main as on my branch, but tests fail in the first place, so i'm not too sure how much i should trust them.

let me know if theres anything else i need to do, or other repos i need to check and update.

[elee1766]

as an aside, i seriously recommend that you consider making each method in datastar.go return errors, instead of eating them and printing or panicking.

Point being is if this fails it's 💯 a dev error and should be immediate

returning an error is failing immediately in go. panic/exception flow is not meant to be used to "fail". Don't Panic is the proverb which comes to mind. You return the error and handle the error all the way up, and the caller can decide how they want to fail (kill the program? kill the connection instance? log and do nothing?).

I tried using datastar personally, and i had to just not use any of your sse code in my server, since it panics (which makes catching errors hard), and i can't handle some errors at all because they get printed. I don't think i'm alone here on the go developer side. on the bright side, the datastar protocol is simple enough that it's easy to implement on top of existing golang sse server implementations.

[delaneyj]

I've already said if it fails it's :100: developer err, there is no way in practice to make it fail the assert, if so please show it happening in a test case.

[elee1766]

you literally say it yourself - in a comment above the code.

        // Can happen if the client closes the connection or
        // other middleware panics during flush (such as compression)
        // Not ideal, but we can't do much about it
        if r := recover(); r != nil && sse.shouldLogPanics {

client closing connection, or another middleware failing, is something that I as the developer need to (and should) be handling. you even say you can't do much about it - admitting that you could be, and are chosing not to.

my patch makes it possible to handle these errors, though you should be handling them too within your datastar package, instead of throwing.

but even if you discount these 100% not developer error that has to be handled that you are not handling - it is still wrong to be returning an error in the middle of code like you are, nearly always.

even in your cited sqlite - if you use the package, you will notice the query/exec functions error on improper queries error (that are not "incorrect" by prep semantics), or have a network error, do not panic. the Prep query panics because it is meant to called on just the query and can be statically checked- not the network send, and so fail-fast-fail-early - you should notify the programmer that their prep failed as soon as possible.

sending data over the wire is nowhere near the same as preparing an sql query or compiling a regex expression. to not handle those errors is not good. you are completely missing the point of that article.

[delaneyj]

You are just incorrect... https://github.com/zombiezen/go-sqlite/blob/ac68309514835610d4ec5c620278cbc5ff09003d/sqlite.go#L438

Please show me backend code using the given code that will cause a panic. The recover is not from MY code, its actually from the compression handler.

[elee1766]

As i said, Prep panics, but using Send/Query does not. I don't understand what you are linking me.

i don't exactly know what you mean by "that recover is not from my code" - it's right here: https://github.com/delaneyj/datastar/blob/main/sse.go#L81-L92 and is the topic of this PR......

if i prepare a query that is valid - and then send it to the server, and either get a network error, lock error, who knows, i am returned an error to deal with

Please show me backend code using the given code that will cause a panic. The recover is not from MY code, its actually from the compression handler

my point is i do not want your code recovering or to eat the errors. i want to handle the errors that you are panic+recovering from (with this, just printing), instead of you handling your own panics and then print+eating them. runtime code should not panic. the code is bad because it is handling exceptions through panics instead of errors, and so it should be corrected to do so with errors, and also it eats up errors, printing, and not propogating them up. I would prefer to not have to use my own sse server in order to correctly handle network errors.

largely that is what this pr allows me to do, since I can use Send instead of MustSend now, except if i want to use the other helpers, they all use MustSend. this means to properly handle a error from a send from a helper, i would need to reimplement it, since you eat the error away, and you can't extract the original error from the panic handler easily since you've lost the errors underlying type

if you don't want to it's fine. If you want your package to handle errors through throwing on send, it's your package i guess.

[delaneyj]

It's recovering compression panic... Not mine

On Sun, Nov 3, 2024, 10:12 AM a @.***> wrote:

As i said, Prep panics, but using Send/Query does not. I don't understand what you are linking me.

Please show me backend code using the given code that will cause a panic. The recover is not from MY code, its actually from the compression handler

my point is i do not want your code recovering. i want to handle the errors that you are recovering from, instead of handling panics. runtime code should not panic,

— Reply to this email directly, view it on GitHub https://github.com/delaneyj/datastar/pull/153#issuecomment-2453516475, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADK73CHQL4X63HCGDX7VYLZ6ZKO3AVCNFSM6AAAAABQA4AUPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJTGUYTMNBXGU . You are receiving this because you commented.Message ID: @.***>

[elee1766]

this pr changes it so it no longer is handling your panics yes - but the error is still lost when using the datastar helpers, since they call MustSend.

[delaneyj]

Please check out the temp repo as we move to v1... https://github.com/starfederation/datastar/tree/main/code/go/sdk

is should address most of your concerns