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

Implement closeRead/closeWrite using TcpStream::shutdown #903

Merged
merged 5 commits into from
Oct 5, 2018
Merged

Implement closeRead/closeWrite using TcpStream::shutdown #903

merged 5 commits into from
Oct 5, 2018

Conversation

kevinkassimo
Copy link
Contributor

@kevinkassimo kevinkassimo commented Oct 4, 2018 •
edited
Loading

Attempt to implement closeRead/closeWrite
Simulated shutdown(2) interface.

Also briefly implemented Conn.addr()

ry
ry reviewed Oct 4, 2018
View reviewed changes
js/net.ts Show resolved Hide resolved
ry
ry reviewed Oct 4, 2018
View reviewed changes
js/net_test.ts Show resolved Hide resolved
ry
ry reviewed Oct 4, 2018
View reviewed changes
src/resources.rs
None => panic!("bad rid"),
Some(repr) => match repr {
Repr::TcpStream(ref mut f) => TcpStream::shutdown(f, how).unwrap(),
_ => panic!("Cannot shutdown"),
Copy link
Member

Choose a reason for hiding this comment

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

Looks good - nice.

@kevinkassimo kevinkassimo changed the title [WIP] Implement closeRead/closeWrite using TcpStream::shutdown & implement conn.addr() Implement closeRead/closeWrite using TcpStream::shutdown & implement conn.addr() Oct 4, 2018
ry
ry reviewed Oct 5, 2018
View reviewed changes
src/resources.rs Outdated
@@ -79,6 +81,21 @@ impl Resource {
let r = table.remove(&self.rid);
assert!(r.is_some());
}

// no collision with unimplemented shutdown
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean?

ry
ry reviewed Oct 5, 2018
View reviewed changes
js/net_test.ts
}
assert(!!err);
assertEqual(err.kind, deno.ErrorKind.NotConnected);
assertEqual(err.name, "NotConnected");
Copy link
Member

Choose a reason for hiding this comment

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

Nice. Thank you.

ry
ry reviewed Oct 5, 2018
View reviewed changes
js/net_test.ts Outdated
conn.close();
});

testPerm({ net: true }, async function netConnDupCloseWriteFailure() {
Copy link
Member

Choose a reason for hiding this comment

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

Please rename to netDoubleCloseWrite()

"dup" has special meaning in this context (see dup(2))

ry
ry reviewed Oct 5, 2018
View reviewed changes
js/net_test.ts Outdated
conn.close();
});

testPerm({ net: true }, async function netConnDupCloseReadFailure() {
Copy link
Member

Choose a reason for hiding this comment

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

Rename to netDoubleCloseRead

ry
ry reviewed Oct 5, 2018
View reviewed changes
js/net_test.ts Outdated
@@ -35,3 +35,115 @@ testPerm({ net: true }, async function netDialListen() {
listener.close();
conn.close();
});

testPerm({ net: true }, async function netConnCloseRead() {
Copy link
Member

Choose a reason for hiding this comment

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

Rename to netCloseReadSuccess()

ry
ry reviewed Oct 5, 2018
View reviewed changes
js/net_test.ts Outdated
conn.close();
});

testPerm({ net: true }, async function netConnCloseWrite() {
Copy link
Member

Choose a reason for hiding this comment

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

Rename to netCloseWriteSuccess

ry
ry reviewed Oct 5, 2018
View reviewed changes
js/net.ts Outdated
export interface Addr {
network: Network;
address: string;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you please leave the Addr changes out of this PR? I'd like to discuss them separately from shutdown.

ry
ry reviewed Oct 5, 2018
View reviewed changes
Copy link
Member

@ry ry left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks good - just a few comments

ry
ry reviewed Oct 5, 2018
View reviewed changes
js/net.ts Outdated Show resolved Hide resolved
ry
ry reviewed Oct 5, 2018
View reviewed changes
js/test_util.ts Outdated Show resolved Hide resolved
@kevinkassimo kevinkassimo changed the title Implement closeRead/closeWrite using TcpStream::shutdown & implement conn.addr() Implement closeRead/closeWrite using TcpStream::shutdown Oct 5, 2018
ry
ry reviewed Oct 5, 2018
View reviewed changes
js/net_test.ts
@@ -1,7 +1,8 @@
// Copyright 2018 the Deno authors. All rights reserved. MIT license.

import * as deno from "deno";
import { testPerm, assert, assertEqual, deferred } from "./test_util.ts";
import { testPerm, assert, assertEqual } from "./test_util.ts";
import { deferred } from "./util.ts";
Copy link
Member

@ry ry Oct 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Cool! I forgot about this when I suggested it - but this is only newly possible because of your work in #859.

js/net_test.ts runs inside of Deno, but js/util.ts runs inside of Node/rollup. But now we are able to seamlessly load util.ts in Deno too.

cc @kitsonk - can compiler_test.ts work like this too?

ry
ry approved these changes Oct 5, 2018
View reviewed changes
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 941e27d into denoland:master Oct 5, 2018
ry added a commit to ry/deno that referenced this pull request Oct 12, 2018
@ry
v0.1.8
73f623f 
- Add --types command line flag.
- Add metrics()
- Add redirect follow feature denoland#934
- Fix clearTimer bug denoland#942
- Improve error printing denoland#935
- Expose I/O interfaces Closer, Seeker, ReaderCloser, WriteCloser,
  ReadSeeker, WriteSeeker, ReadWriteCloser, ReadWriteSeeker
- Fix silent death on double await denoland#919
- Add Conn.closeRead() and Conn.closeWrite() denoland#903
@ry ry mentioned this pull request Oct 12, 2018
Merged
ry added a commit to ry/deno that referenced this pull request Oct 12, 2018
@ry
v0.1.8
6587a18 
- Fix promise reject issue (denoland#936)
- Add --types command line flag.
- Add metrics()
- Add redirect follow feature denoland#934
- Fix clearTimer bug denoland#942
- Improve error printing denoland#935
- Expose I/O interfaces Closer, Seeker, ReaderCloser, WriteCloser,
  ReadSeeker, WriteSeeker, ReadWriteCloser, ReadWriteSeeker
- Fix silent death on double await denoland#919
- Add Conn.closeRead() and Conn.closeWrite() denoland#903
ry added a commit that referenced this pull request Oct 12, 2018
@ry
v0.1.8
d92c99e 
- Fix promise reject issue (#936)
- Add --types command line flag.
- Add metrics()
- Add redirect follow feature #934
- Fix clearTimer bug #942
- Improve error printing #935
- Expose I/O interfaces Closer, Seeker, ReaderCloser, WriteCloser,
  ReadSeeker, WriteSeeker, ReadWriteCloser, ReadWriteSeeker
- Fix silent death on double await #919
- Add Conn.closeRead() and Conn.closeWrite() #903
@kevinkassimo kevinkassimo deleted the net/shutdown branch December 27, 2019 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry ry ry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kevinkassimo @ry