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

Only return errors, without panics #439

Open
agamb-cerebras opened this issue Feb 27, 2025 · 4 comments
Open

Only return errors, without panics #439

agamb-cerebras opened this issue Feb 27, 2025 · 4 comments
Labels
enhancement New feature or request

Comments

@agamb-cerebras
Copy link

Is your feature request related to a problem? Please describe.

Hi all,

I've been using the go-mail library and have found it very helpful.

However, I've encountered some scenarios where the library uses panic for errors that could potentially be handled gracefully by the calling code.

This can lead to unexpected application termination (!)

As a result, I have to wrap calls with a recover(...) block. I feel the interface of the library should not push through panics this way, and should represent such states as new error codes instead.


Here's an example of how this fails, when sending mail:

panic: out of sync

goroutine 75 [running]:
net/textproto.(*sequencer).End(0xc0009104e0, 0xc0008a0b08?)
	/usr/local/go/src/net/textproto/pipeline.go:103 +0x145
net/textproto.(*Pipeline).EndResponse(0x13c9fa0?, 0xc000910401?)
	/usr/local/go/src/net/textproto/pipeline.go:65 +0x19
github.com/wneessen/go-mail/smtp.(*Client).cmd(0xc000816000, 0x1a?, {0x15d2323?, 0x3?}, {0xc000594320?, 0x1392760?, 0xc00024ec38?})
	/app/vendor/github.com/wneessen/go-mail/smtp/smtp.go:139 +0x392
github.com/wneessen/go-mail/smtp.(*Client).Rcpt(0xc000816000, {0xc00004a140, 0x1a})
	/app/vendor/github.com/wneessen/go-mail/smtp/smtp.go:287 +0x1a5
github.com/wneessen/go-mail.(*Client).Send(0xc0006a8000, {0xc0008a11f8, 0x1, 0xd8?})
	/app/vendor/github.com/wneessen/go-mail/client_120.go:63 +0xdc5
github.com/wneessen/go-mail.(*Client).DialAndSendWithContext(0xc0006f4000?, {0x1800d60?, 0x22c0ec0?}, {0xc0008a11f8, 0x1, 0x1})
	/app/vendor/github.com/wneessen/go-mail/client.go:663 +0x8d
github.com/wneessen/go-mail.(*Client).DialAndSend(...)
	/app/vendor/github.com/wneessen/go-mail/client.go:654
<package/app info redacted>.(*mailClient).SendMail(0xc0006945b8, {0xc00069c2c0, 0x4, 0x4}, {0xc0000482a0, 0x11}, {0xc000022000, 0xd8})
... 
... snipped away other application code stack info
...
2025/02/27 16:17:55 running command: exit status 2

Describe the solution you'd like

I would prefer that this recover(...) logic was handled internally within go-mail, so that its interface can be used in a uniform idiomatic if ..., err := ...; err != nil ... etc way.

Of course, this is a matter of personal preference too, and if this has already been considered and rejected, please share the reason and close this out instead -- otherwise, please do consider adopting this approach.

Describe alternatives you've considered

No response

Additional context

No response

@agamb-cerebras agamb-cerebras added the enhancement New feature or request label Feb 27, 2025
@wneessen
Copy link
Owner

Hi @agamb-cerebras, thanks for the report.

go-mail does not use panic anywhere in the code, so that error must be coming from somewhere in the Go standard library. Could you share some code to reproduce, so I can have a look at what might be the root cause?

@agamb-cerebras
Copy link
Author

agamb-cerebras commented Feb 27, 2025

Yes, it's not from within go-mail itself; I added a partial panic-stacktrace above, which shows that the actual panic comes from the low-level net/textproto library.

I'm going to add logging with my recover(...) wrapper, which should hopefully yield examples of targets that I can share here.

[fwiw, I suspect this happened with something like [email protected]]

@wneessen
Copy link
Owner

Btw. which version of go-mail are you using. I checked the stack trace and the lines reported do not match up with the current code.

@wneessen
Copy link
Owner

I did some digging in the textproto code and found the potential panic. It's issued in the End method of the sequencer. It panics if the sequencer id does not match the end id. This comes likely down to a concurrency issue. I assume it is a similar issue to #385.

Given the code in the stacktrace I assume you are not using go-mail v0.6+ yet. The concurrency issue was fixed in v0.6.0 (PR #386). If this is the case, I suggest you update to the latest release and check if the panics still occur.

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

No branches or pull requests

2 participants