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

Implement .native-descriptor for async sockets #2344

Merged
merged 2 commits into from Oct 20, 2018

Conversation

Projects
None yet
3 participants
@Kaiepi
Copy link
Contributor

Kaiepi commented Oct 6, 2018

This will allow users to get the native descriptor of the sockets later
on. See perl6/nqp#505 and MoarVM/MoarVM#980

@Kaiepi Kaiepi force-pushed the Kaiepi:async-socket-create branch 3 times, most recently from f76d0ff to af9f55d Oct 6, 2018

@Kaiepi

This comment has been minimized.

Copy link
Contributor Author

Kaiepi commented Oct 6, 2018

At some point while I was working on this, S32-io/IO-Socket-Async.t stopped passing. I'll fix it as soon as possible

@Kaiepi Kaiepi force-pushed the Kaiepi:async-socket-create branch from 21b3b1c to 231a747 Oct 8, 2018

@Kaiepi

This comment has been minimized.

Copy link
Contributor Author

Kaiepi commented Oct 8, 2018

This now passes all tests (ignoring Travis/AppVeyor not being aware of the asyncsocket op yet)

@Kaiepi Kaiepi force-pushed the Kaiepi:async-socket-create branch from 231a747 to 58b0e27 Oct 8, 2018

Create async sockets before passing them to .listen/.connect
This will allow users to get the native descriptor of the sockets later
on.

@Kaiepi Kaiepi force-pushed the Kaiepi:async-socket-create branch from 58b0e27 to a1a95e0 Oct 13, 2018

@Kaiepi Kaiepi changed the title Create async sockets before passing them to .listen/.connect Implement .native-descriptor for async sockets Oct 15, 2018

@Kaiepi

This comment has been minimized.

Copy link
Contributor Author

Kaiepi commented Oct 15, 2018

Could I get a review on this and the other pullreqs now that the work is done?

@@ -122,7 +122,8 @@ my class IO::Socket::Async {
nqp::decont($!buf), SocketCancellation);
$tap := Tap.new({ nqp::cancel($cancellation) });
tap($tap);
}
};

This comment has been minimized.

Copy link
@ugexe

ugexe Oct 16, 2018

Member

Unneeded addition of ;

Show resolved Hide resolved src/core/IO/Socket/Async.pm6
$v.break: err;
} else {
my \client = nqp::create(self);
my $encoding = Encoding::Registry.find($enc);

This comment has been minimized.

Copy link
@ugexe

ugexe Oct 16, 2018

Member

Extra non-aligning spaces used between variable name and =

:$socket-host, :$socket-port)) unless $tap;
$tap = ListenSocket.new({ Nil }, :$VMIO,
:$socket-host, :$socket-port) unless $tap;
tap($tap);

This comment has been minimized.

Copy link
@ugexe

ugexe Oct 16, 2018

Member

I can't tell if this is intended or not, but the logic is not quite the same anymore.
Before:

unless $tap {
    tap($tap = ...)
}

After:

unless $tap {
    $tap = ...;
}
tap($tap)

This comment has been minimized.

Copy link
@Kaiepi

Kaiepi Oct 16, 2018

Author Contributor

Unintended

@Kaiepi Kaiepi force-pushed the Kaiepi:async-socket-create branch from dcf747c to dd0cb27 Oct 16, 2018

CATCH {
default {
tap($tap = ListenSocket.new({ Nil },
:$socket-host, :$socket-port)) unless $tap;
tap(ListenSocket.new: { Nil }, :$VMIO,

This comment has been minimized.

Copy link
@ugexe

ugexe Oct 16, 2018

Member

This is still not equivalent to the previous code, which assigned the ListenSocket to $tap

This comment has been minimized.

Copy link
@Kaiepi

Kaiepi Oct 17, 2018

Author Contributor

Oop

@Kaiepi Kaiepi force-pushed the Kaiepi:async-socket-create branch from dd0cb27 to 4d3794b Oct 17, 2018

self.bless(:&on-close, :$socket-host, :$socket-port);
submethod BUILD(Mu :$!VMIO, Promise :$!socket-host, Promise :$!socket-port) { }

method new(&on-close, Mu :$VMIO, Promise :$socket-host, Promise :$socket-port) {

This comment has been minimized.

Copy link
@ugexe

ugexe Oct 18, 2018

Member

Should $VMIO be required ala Mu :$VMIO!? Similar to

method new(Mu :$VMIO!, :$scheduler!, :$buf!, :$close-promise!, :$udp!) {

This comment has been minimized.

Copy link
@Kaiepi

Kaiepi Oct 19, 2018

Author Contributor

I don't see why not

@Kaiepi Kaiepi force-pushed the Kaiepi:async-socket-create branch from 4d3794b to 91e583f Oct 19, 2018

@Kaiepi Kaiepi force-pushed the Kaiepi:async-socket-create branch from 91e583f to 865b1f3 Oct 19, 2018

@ugexe

ugexe approved these changes Oct 19, 2018

@zoffixznet zoffixznet merged commit 1ac944f into rakudo:master Oct 20, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@zoffixznet

This comment has been minimized.

Copy link
Contributor

zoffixznet commented Oct 20, 2018

👍 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.