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

REFACTOR: Use Async and family #467

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

Conversation

Ahmedgagan
Copy link
Contributor

@Ahmedgagan Ahmedgagan commented Jun 28, 2021

Hi @sj26
This PR is on behalf of Discourse.
This PR replaces Eventmachine with Async.

@SamSaffron
Copy link

Howdy @sj26, anything we can do to help drive this forward? Do you need any assistance reviewing, I could ask another person from our team to carefully review.

@sj26
Copy link
Owner

sj26 commented Jul 6, 2021

Hi folks! This looks amazing, thanks for the contribution. I haven't had capacity to review this properly yet, and the changes are very extensive. It also looks like there are a few functional changes in here beyond refactoring which will need to be considered. I'll see if I can start dropping some smaller comments. (And I was leaving time for the "more changes" 🙂)

else
puts "*** MailCatcher is now running as a daemon that cannot be quit."
end
Process.daemon
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was daemonisation removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding it in the next commit. Keeping it in the async block was blocking the process instead of daemonisation. I got the solution for this 😅.

def initialize
super
@connections = Set.new
@semaphore = Async::Semaphore.new(512)
Copy link
Owner

@sj26 sj26 Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supervising websockets and acting as a message bus? Perhaps that's a separate concern from the core web application, and should be extracted?

I'm not averse to pulling in something like MessageBus to do this well, if there's dependency alignment. Or building an equivalent little bus in here using async. I've been meaning to generalise MailCatcher's websocket-y message bus to keep better state between the backend and frontend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the semaphores, it was not of use for our application. I think Async-WebSockets do well for our case. thoughts?

def run
@endpoint.accept(&method(:accept))
end
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing, a whole SMTP implementation! If we're cooking our own then I think we'll need to add some testing.

Copy link
Owner

@sj26 sj26 Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Ah yes, it looks familiar: e77365b)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I used your async branch and made it work with the master branch's latest changes 😀

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

We'll need to test the implementation. How would you like to do that? If minitest feels cubersome then I'm happy to switch to rspec if it's more familiar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I am more familiar with RSpec, I'll start with converting to RSpec tests


connection.write data
connection.flush
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this bit about, is this constructing a websocket client to itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have used Async Websocket gem which needs to create a client to itself to send data. you can see the example here

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I don’t think we should do that. There should be a way to push a message to clients without being a client. And clients probably shouldn’t be able to send each other messages.

Copy link
Contributor Author

@Ahmedgagan Ahmedgagan Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some research and I think there's no way to push a message without the connection object which we get from the connection to WebSocket itself. I improved connection handling in the latest commit

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a new version which uses a central bus for broadcasting and ignores client messages as I was describing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I just saw that. before I tried using queue but unfortunately I was not getting the connections. Your implementation looks damn good 😍 Thanks for the implementation 😄

lib/mail_catcher.rb Outdated Show resolved Hide resolved
@Ahmedgagan
Copy link
Contributor Author

Hi @sj26
Last week I made some commits as per your suggestions, Do have a look when you get time.

sj26 and others added 4 commits July 13, 2021 11:38
mailcatcher --verbose failed with:

	NameError: uninitialized constant MailCatcher::Logger

Async.logger is a Console::Logger, not a Logger. But it accepts symbols
as log levels.
Add a Bus which is a Channel modelled roughly after eventmachine
channels which can broadcast a message to multiple consumers as async
tasks. Then modify each websocket to use a Queue to copy broadcast
messages to clients.
@http_endpoint = Async::IO::AddressEndpoint.new(@http_address)
@http_socket = rescue_port(options[:http_port]) { @http_endpoint.bind }
puts "==> #{http_url}"
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, nice.

@sj26
Copy link
Owner

sj26 commented Jul 13, 2021

I've merged a switch to GitHub Actions to main so that there is some form of CI:

#469

I'm not set on using mintest or even selenium (https://github.com/rubycdp/cuprite looks great!) but the fundamental infrastructure is available (ruby, node, chrome, etc).

@Ahmedgagan
Copy link
Contributor Author

Hi @sj26
I have added a commit to convert Mintest to RSpec and All tests are passed(unless skipped) in my local. Starting to write SMTP implementation spec.

Also, If you wish to check Github CI setup works well with RSpec, Just approve this PR and GitHub actions will be performed on all my next commits

write_response 501, 'Unexpected parameters or arguments'
next
end
envelope = nil
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
envelope = nil
@state.clear

@Ahmedgagan
Copy link
Contributor Author

Hi @sj26 ,
I have added RSpec for smtp.rb file, if possible do have a look

@sj26
Copy link
Owner

sj26 commented Jul 20, 2021

Thanks @Ahmedgagan. The formatting of that file is a bit random, tabs and spaces etc, but I'll peer between that for the moment.

I can see it's testing some of the plumbing of the line protocol stuff. I'm not so interested in that, and more interested in testing that we're doing correct SMTP protocol interactions. Like can a new connection EHLO and receive an expected response. Does the MAIL FROM: ... command parse the sender correctly, and return expected responses in various cases. Does the server enforce a correct ordering of EHLO, MAIL FROM, RCPT TO, DATA and error correctly when done out of order. Do the encodings work — does it support 7bit transfer correctly, and utf8. Because this is a completely novel SMTP implementation I think these details are important. [edit: testing the dot stuffing behaviour is super important too, the . behaviour in DATA]

By SMTP clients I do indeed mean testing it manually from different environments, not just ruby's Net::SMTP. Like does it work from Django, or from Laravel, or other environments where devs are likely to use it. I agree that releasing a beta is a good way to do that too, but I don't want to drop such a beta without doing at least a little bit of independent testing first.

I didn't test if (m)any clients use BDAT before implementing it. If we can achieve all the required client support without it then I would consider removing it to simplify the implementation and testing. I think I only added it while playing with an SMTP server implementation and exploring ways to ingest emails with full octet encodings which have historically been a pain in MailCatcher's side. [edit: see later comment, I think we should remove CHUNKING/BDAT.]

So, a roadmap to getting this merged:

  1. I need to write some more feature specs for MailCatcher to test things like attachments, downloading parts, Clear, Quit, and search. They should continue to pass on this branch.
  2. This branch needs to be ruby 3.0 compatible. I noticed there are some errors/timeouts on CI around async tasks and ruby 3.
  3. We need tests for the SMTP implementation to ensure correct implementation of the commands, and to ensure sequencing works as expect with appropriate responses. Encodings should also be carefully tested.
  4. We need to do some manual testing using various SMTP client libraries which are typically used in popular development environments.

@sj26
Copy link
Owner

sj26 commented Jul 20, 2021

Most of them seem to support 8BITMIME and SMTPUTF8.

None support CHUNKING (BDAT) so I think we should rip that out.

@Ahmedgagan
Copy link
Contributor Author

Hi @sj26
I made it work with ruby 3.0, and also added some more spec to test SMTP protocol interactions. Do share your thoughts.

Now, I am starting with testing it in different frameworks.

@Ahmedgagan
Copy link
Contributor Author

Ahmedgagan commented Jul 26, 2021

Hi @sj26
I tested the SMTP implementation in python, Ruby, Golang & PHP. It worked in all 4 perfectly, there was a small problem with python in which it was sending commands in lowercase & our switch case was comparing uppercase strings. This commit solves it. I tested with sending plain text, HTML, attachments, attaching images in mail content, etc.

@Ahmedgagan
Copy link
Contributor Author

So, a roadmap to getting this merged:

  1. I need to write some more feature specs for MailCatcher to test things like attachments, downloading parts, Clear, Quit, and search. They should continue to pass on this branch.
  2. This branch needs to be ruby 3.0 compatible. I noticed there are some errors/timeouts on CI around async tasks and ruby 3.
  3. We need tests for the SMTP implementation to ensure correct implementation of the commands, and to ensure sequencing works as expect with appropriate responses. Encodings should also be carefully tested.
  4. We need to do some manual testing using various SMTP client libraries which are typically used in popular development environments.

Hi @sj26
I think I have completed points 2,3,4. Can you have a look?

spec/smtp_protocol_spec.rb Outdated Show resolved Hide resolved
@sj26
Copy link
Owner

sj26 commented Jul 28, 2021

Thanks for testing it across implementations @Ahmedgagan!

Sorry it's taken me a while to take a look. This isn't my day job, so I have to steal time where I can.

These specs look a little light to me. There's lots of functionality which is untested, and quite a few status codes which aren't exercised. I'll try to dig in and offer some more concrete suggestions or add some commits this weekend.

I did manage to write quite a few more feature specs last weekend, but I haven't pushed them up yet. I wanted to get 0.8.0 out and stable with the improvements which had been collecting on the main branch.

@Ahmedgagan
Copy link
Contributor Author

Sorry it's taken me a while to take a look. This isn't my day job, so I have to steal time where I can.

ooh, thanks for the efforts 😅

These specs look a little light to me. There's lots of functionality which is untested, and quite a few status codes which
aren't exercised. I'll try to dig in and offer some more concrete suggestions or add some commits this weekend.

I'll also try to write some more concrete tests.

I did manage to write quite a few more feature specs last weekend, but I haven't pushed them up yet. I wanted to get
0.8.0 out and stable with the improvements which had been collecting on the main branch.

That's great, after the release we will merge those stable changes in this branch.

Co-authored-by: Samuel Cochran <sj26@sj26.com>
@Ahmedgagan
Copy link
Contributor Author

Ahmedgagan commented Aug 9, 2021

I'll try to dig in and offer some more concrete suggestions or add some commits this weekend.

Hi @sj26
How are you? Can you have a look at the PR?

@SamSaffron
Copy link

Howdy @sj26 hope you are doing well! Anything we can do to move the merge forward.

We already had multiple people on ARM macs test this and it appears to be working nicely!

@SamSaffron
Copy link

@sj26 any news? Do you need a hand with the gem, I can ask around if any of our senior devs is open to helping maintain

@sj26
Copy link
Owner

sj26 commented Oct 12, 2021

@Ahmedgagan @SamSaffron sorry folks, I haven't had capacity to come back and do the project managementy bits. The world is not kind at the moment. I love this work, but I want it to land nicely and remain maintainable.

I'd like some more feature tests to make sure it's not breaking anything fundamental that folks currently use (working on main, and continuing to work on this branch), more test coverage of the novel smtp implementation's edges to make sure it doesn't regress, and I'd like it to match the code style a little more (it's silly but I'd prefer to keep to double quotes like the existing code, for example).

I think there is also a depth of edges which need careful review. For example, I think the new quit implementation currently just stops the reactor cold instead of sending a quit message to clients and ending the reactor after current connections are closed and finished (which the eventmachine implementation used to do by pushing to the bus and then stopping on the next tick). It needs a careful comb through.

If you have capacity to do these things, or know folks who can, that would be grand. Otherwise I'll get to it when I can.

@sj26 sj26 linked an issue Nov 1, 2021 that may be closed by this pull request
thenonameguy added a commit to thenonameguy/nixpkgs that referenced this pull request Dec 1, 2022
Fixes NixOS#96908

Switching to fork that uses async library, this fixes darwin builds.
sj26/mailcatcher#467
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants