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

Protocol specific IOLoop streams #1227

Merged
merged 1 commit into from
Jun 1, 2018
Merged

Protocol specific IOLoop streams #1227

merged 1 commit into from
Jun 1, 2018

Conversation

anparker
Copy link
Contributor

@anparker anparker commented May 29, 2018 •
edited
Loading

Summary

Implementing Mojo::IOLoop::Stream subclasses for HTTP and WebSocket connections.

Motivation

An attempt to implement feature required for an issue.

References

#423 (comment)

kraih
kraih requested changes May 29, 2018
View reviewed changes
lib/Mojo/IOLoop.pm Outdated
Mojo::IOLoop->remove($id)

Perform transition of a stream container class to a new one. It will steal handle
from an original connection, but won't remove connection itself.
Copy link
Member

Choose a reason for hiding this comment

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

Description is bad english.

lib/Mojo/IOLoop.pm Outdated

my $new = Mojo::IOLoop->transition($id, 'Mojo::IOLoop::Stream::HTTPClient');
my $stream = Mojo::IOLoop->stream($new);
Mojo::IOLoop->remove($id)
Copy link
Member

Choose a reason for hiding this comment

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

The example is inconsistent with surrounding examples.

lib/Mojo/IOLoop/Stream/Client.pm Outdated

=head1 DESCRIPTION

Base class for building transaction based protocol specific clients.
Copy link
Member

Choose a reason for hiding this comment

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

The description sounds like a generic placeholder.

lib/Mojo/IOLoop/Stream/Client.pm Outdated
}
);
$stream->start;
$stream->process($tx);
Copy link
Member

Choose a reason for hiding this comment

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

Example is inconsistent with other method examples in Mojolicious.

lib/Mojo/IOLoop/Stream/Client.pm Outdated

=cut

# vim: tabstop=2 softtabstop=2 expandtab shiftwidth=2 smarttab tw=81 fo+=t fo-=l
Copy link
Member

Choose a reason for hiding this comment

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

We don't keep vim lines in Mojolicious.

t/mojo/streams.t Outdated
ok !$stash->{kept_alive}, 'connection was not kept alive';
ok !$closed, 'connection is not closed';

$app->plugins->once(before_dispatch => sub { $stash = shift->stash });
Copy link
Member

Choose a reason for hiding this comment

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

This block of code looks lost.

t/mojo/streams.t Outdated
$tx = _tx('http://127.0.0.1/keep-alive');
$tx->on(finish => sub { Mojo::IOLoop->stop });

$client_stream->process($tx);
Copy link
Member

Choose a reason for hiding this comment

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

This block looks lost too.

t/mojo/streams.t Outdated
Mojo::IOLoop->stop;
}
);

Copy link
Member

Choose a reason for hiding this comment

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

Extra newline.

t/mojo/streams.t Outdated
return $t;
}

# vim: tabstop=2 softtabstop=2 expandtab shiftwidth=2 smarttab tw=81 fo+=t fo-=l
Copy link
Member

Choose a reason for hiding this comment

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

We do not keep vim lines in Mojolicious.

t/mojo/streams.t Outdated
is $err->{message}, 'Request timeout', 'right message';


done_testing();
Copy link
Member

Choose a reason for hiding this comment

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

We usually make done_testing(); the last line in a test.

@kraih
Copy link
Member

kraih commented May 29, 2018 •
edited
Loading

For those not following IRC, i was the one who recommended the change from MOJO_USERAGENT_DEBUG and MOJO_DAEMON_DEBUG to MOJO_CLIENT_DEBUG and MOJO_SERVER_DEBUG. Since the debug code would have to span multiple classes on different layers after this pull request, it just makes more sense this way.

kraih
kraih reviewed May 29, 2018
View reviewed changes
lib/Mojo/IOLoop.pm Outdated
@@ -48,6 +48,9 @@ sub acceptor {
sub client {
my ($self, $cb) = (_instance(shift), pop);

Copy link
Member

Choose a reason for hiding this comment

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

Extra newline. Don't split up arg handling if you don't have to.

kraih
kraih reviewed May 29, 2018
View reviewed changes
lib/Mojo/IOLoop.pm Outdated
@@ -106,11 +109,14 @@ sub reset {
sub server {
my ($self, $cb) = (_instance(shift), pop);

Copy link
Member

Choose a reason for hiding this comment

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

Extra newline. Don't split up arg handling if you don't have to.

kraih
kraih reviewed May 29, 2018
View reviewed changes
lib/Mojo/IOLoop.pm Outdated
sub transition {
my ($self, $id, $class) = (_instance(shift), @_);
my $new = $class->new($self->stream($id)->steal_handle);
return $self->_stream($new, $self->_id, !!$self->{in}{$id});
Copy link
Member

Choose a reason for hiding this comment

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

How is the old stream garbage collected? Can't we just reuse the same id? Once the handle is stolen the old stream is dead anyway.

@anparker anparker force-pushed the http_stream branch 2 times, most recently from 25ea38e to 04067a5 Compare May 30, 2018 20:54
kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop.pm Outdated

my $stream = Mojo::IOLoop->transition($id, 'Mojo::IOLoop::Stream::HTTPClient');

Replace connection with new one from different class.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something simple like Transition stream to a different class. would be better?

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/HTTPClient.pm Outdated

=head1 NAME

Mojo::IOLoop::Stream::HTTPClient - Non-blocking I/O HTTP client
Copy link
Member

Choose a reason for hiding this comment

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

I think Non-blocking I/O HTTP client stream would work better with the theme.

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/HTTPClient.pm Outdated
}
);

$client->connect(address => 'mojolicious.org', port => 80);
Copy link
Member

@kraih kraih May 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This block needs a descriptive comment too, or it looks strange.

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/HTTPClient.pm Outdated
$tx->on(finish => sub {
my $tx = shift;
say $tx->res->code;
});
Copy link
Member

@kraih kraih May 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Borked indentation.

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/HTTPClient.pm Outdated

=head1 DESCRIPTION

HTTP client connection based on L<Mojo::IOLoop::Stream>.
Copy link
Member

Choose a reason for hiding this comment

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

Needs a better description.

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/HTTPClient.pm Outdated
...
});

Emitted when connection should be upgraded to WebSocket.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe Emitted when the connection should be upgraded to the WebSocket protocol.?

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/HTTPClient.pm Outdated

$stream->process(Mojo::Transaction::HTTP->new);

Process a transaction with current connection.
Copy link
Member

@kraih kraih May 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe Process a L<Mojo::Transaction::HTTP> object with the current connection?

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/HTTPServer.pm Outdated
$tx->on(resume => sub { $self->_write_content });
$self->_write_content;
}

Copy link
Member

Choose a reason for hiding this comment

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

Extra newline.

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/HTTPServer.pm Outdated

=head1 NAME

Mojo::IOLoop::Stream::HTTPServer - Non-blocking I/O HTTP server
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking I/O HTTP server stream?

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/HTTPServer.pm Outdated

=head1 DESCRIPTION

HTTP server connection based on L<Mojo::IOLoop::Stream>.
Copy link
Member

Choose a reason for hiding this comment

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

Needs a better description.

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/HTTPServer.pm Outdated
...
});

Emitted when a transaction for new request was build.
Copy link
Member

@kraih kraih May 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Can't help but think that this event needs a better name, maybe something like start? (Oh, and of course a better description 😉)

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/HTTPServer.pm Outdated
...
});

Emitted when connection should be upgraded to WebSocket.
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, needs a better description.

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/WebSocketClient.pm Outdated

=head1 NAME

Mojo::IOLoop::Stream::WebSocketClient - Non-blocking I/O WebSocket client
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking I/O WebSocket client stream?

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/WebSocketClient.pm Outdated

=head1 DESCRIPTION

WebSocket client connection based on L<Mojo::IOLoop::Stream>.
Copy link
Member

Choose a reason for hiding this comment

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

Better description, as usual. 😄

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/WebSocketServer.pm Outdated

my $stream = Mojo::IOLoop::Stream::WebSocketServer->new($handle);
$stream->tx($ws);

Copy link
Member

Choose a reason for hiding this comment

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

Extra newline.

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/WebSocketServer.pm Outdated
use Mojo::IOLoop::Stream::WebSocketServer;
use Mojo::Transaction::WebSocket;

my $ws = Mojo::Transaction::WebSocket->new;
Copy link
Member

Choose a reason for hiding this comment

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

Code blocks are missing descriptive comments.

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/WebSocketServer.pm Outdated

$stream->process(Mojo::Transaction::WebSocket->new);

Process a transaction with current connection.
Copy link
Member

Choose a reason for hiding this comment

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

Process a L<Mojo::Transaction::WebSocket> object.?

kraih
kraih reviewed May 30, 2018
View reviewed changes
lib/Mojo/IOLoop/Stream/WebSocketClient.pm Outdated

$stream->process(Mojo::Transaction::WebSocket->new);

Process a transaction with current connection.
Copy link
Member

Choose a reason for hiding this comment

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

Process a L<Mojo::Transaction::WebSocket> object.

kraih
kraih reviewed May 31, 2018
View reviewed changes
t/mojo/streams.t Outdated
@@ -0,0 +1,175 @@
use Mojo::Base -strict;

BEGIN { $ENV{MOJO_REQUEST_TIMEOUT} = 0.25 }
Copy link
Member

Choose a reason for hiding this comment

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

What is this used for?

Copy link
Member

@kraih kraih May 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There should also be a BEGIN { $ENV{MOJO_REACTOR} = 'Mojo::Reactor::Poll' } here, to avoid accidental uses of EV.

@kraih kraih requested a review from jberger May 31, 2018 22:44
@kraih kraih changed the title Protocol specific IOLoop streams (work in progress) Protocol specific IOLoop streams Jun 1, 2018
@kraih
Copy link
Member

kraih commented Jun 1, 2018

I've removed the work in progress label, since i think this pull request has reached the state where it could be merged.

@kraih
Copy link
Member

kraih commented Jun 1, 2018

@jberger @marcusramberg @jhthorsen I'm calling for a formal vote on this pull request.

@kraih
Copy link
Member

kraih commented Jun 1, 2018

I've reviewed the code and it looks good. The new stream APIs are very minimalistic and seem future proof. A performance difference is barely measurable. The new transition method in Mojo::IOLoop is not exactly elegant, but it does its job and i can't really think of something better either. So this pull request gets a 👍 from me.

P.S.: I think there might even be more code in Mojo::UserAgent that could be moved into stream classes.

marcusramberg
marcusramberg approved these changes Jun 1, 2018
View reviewed changes
Copy link
Member

@marcusramberg marcusramberg left a comment

Choose a reason for hiding this comment

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

Looks solid

@kraih kraih merged commit 6d276f5 into mojolicious:master Jun 1, 2018
@kraih
Copy link
Member

kraih commented Jun 1, 2018

Thanks, applied.

@kraih kraih mentioned this pull request Jun 3, 2018
Open
@ghost ghost mentioned this pull request Jul 25, 2018
Closed
@kraih
Copy link
Member

kraih commented Aug 17, 2018

Unfortunately this change had to be reverted. 61f6cbf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcusramberg marcusramberg marcusramberg approved these changes

@kraih kraih kraih approved these changes

@jberger jberger jberger approved these changes

@jhthorsen jhthorsen Awaiting requested review from jhthorsen

Assignees
No one assigned
Labels
feature request needs feedback
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anparker @kraih @marcusramberg @jberger