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.

Sign up for GitHub

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

Jump to bottom

Introduce new ConnectionClosed method on Milter interface #17

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Introduce new ConnectionClosed method on Milter interface #17

wants to merge 2 commits into from

Conversation

d--j
Copy link
Owner

@d--j d--j commented Oct 6, 2023

introduce new ConnectionClosed method to inform milter backends when a milter connection was closed

BREAKING CHANGE since added a new method to the Milter interface. If you embedded the milter.NoOpMilter in your own milter backend you should not need to do anything.

// ConnectionClosed gets called after the milter connection was closed (by the client or server).
// It does not get called when client issues a SMFIC_QUIT_NC command or when one connection gets used for multiple messages.
// You should not use this function to clean up resources. Clients can use a single milter connection to process
// multiple SMTP messages and/or SMTP connections. Every message gets a new [Milter] backend but only the last
// [Milter] backend's ConnectionClosed function gets called (when the connection finally gets closed).
//
// The parameter lastCommand is the last command that the client passed to the [Milter].
//
// The optional resp parameter is our response that we sent (err = nil) or would have sent (err != nil, if we already had one).
//
// The optional err parameter is the error that we might have gotten (the reason why the connection closed).
// It might be nil when we closed the connection.
ConnectionClosed(lastCommand ClientCommand, resp *Response, err error)

resolves #13

@d--j
feat: introduce new ConnectionClosed method to inform milter backends…
45322a0 
… when a milter connection was closed

BREAKING CHANGE since added a new method to the Milter interface. If you embedded the milter.NoOpMilter in your own milter backend you should not need to do anything.
@d--j
chore: updated dependencies
24197cd
@coveralls
Copy link
Collaborator

Coverage Status

coverage: 85.752% (-0.04%) from 85.796% when pulling 24197cd on connection into 6cc737f on main.

@d--j d--j mentioned this pull request Oct 6, 2023
Open
@iredmail
Copy link

iredmail commented Oct 6, 2023

Dear @d--j

Thank you very much for the followup.
I'm now testing this PR in production, will get back to you soon.

@iredmail
Copy link

iredmail commented Oct 6, 2023 •
edited
Loading

During my testing, all smtp sessions were one message in one smtp session, so i didn't test one smtp session with multiple messages so far.

  • In my testing, although the final response is milter.RespContinue, the resp *Response in ConnectionClosed is always nil. I expect it to be same response of processed single message (RespContinue in my case). But i have no idea how it should be if one message contains multiple messages, so it might be better to replace ConnectionClosed by something like ProcessedMessage which is called right after processed each message - no matter it's one or more messages in one smtp session. And for logging purpose, it's important to log response of each message, not smtp session.
    • it's possible to introduce a new field in serverSession struct to identify the session, so that we can know whether messages are in same smtp session.
  • Also one possible bug: If resp milter.Response is nil, resp.String() causes panic. I think it's better to return empty string or at least <nil>. It misses default: branch in first switch: https://github.com/d--j/go-milter/blob/main/response.go#L70C3-L70C3

@iredmail
Copy link

iredmail commented Oct 7, 2023

One more bug: backend was reset before calling ConnectionClosed, so session info is gone, only empty field value got logged.

@d--j
Copy link
Owner Author

d--j commented Oct 9, 2023

ConnectionClosed is not the right place for logging your milters actions. Do that in EndOfMessage. ConnectionClosed gives you a chance to log aborts (your milter started processing a message but the MTA did not finish the whole milter filter transaction).

One more bug: backend was reset before calling ConnectionClosed, so session info is gone, only empty field value got logged.

What is the lastCommand parameter in this case?

@iredmail
Copy link

iredmail commented Oct 9, 2023 •
edited
Loading

ConnectionClosed is not the right place for logging your milters actions. Do that in EndOfMessage.

EndOfMessage is not the right place: if message is rejected in Headers (End of headers) or other state before EndOfMessage, it doesn't go through EndOfMessage, so no logging is triggered.

One more bug: backend was reset before calling ConnectionClosed, so session info is gone, only empty field value got logged.

What is the lastCommand parameter in this case?

The last command is QUIT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

What's the best place to log final response?
3 participants
@d--j @coveralls @iredmail