WIP: Add waker to StreamResource

[jsouto18]

This Pull Request serves to present a WIP solution for the issues #4284 and #3390.
I've traced the cause of #4284 to the poll_read task that was getting stuck on Poll::Pending. It was never being woken up due to the already closed underlying connection.
This WIP solution implements the same waking solution used on TcpListenerResource under //cli/ops/net.rs for the StreamResourceHolder struct.

Currently only the net.rs tcp resources function properly.

[jsouto18]

@ry @bartlomieju (review)

[bartlomieju]

@ry, @piscisaureus and I had a discussion about this solution; it looks good on the surface; but it effectively prevents having multiple ops reading from single resource. Although in most cases we only want to have a single op, there are some cases where having multiple ops reading from single resource is desirable.

Ideally we would track multiple tasks for single resource, but Waker is very simple struct that has no PartialEq or any other trait implementation that would allow to discriminate wakers.

[bartlomieju]

We'll discuss it more tonight

[jsouto18]

@bartlomieju I pushed some small changes to the StreamResourceWrapper to make it capable of waking multiple tasks. It just a suggestion to help the discussion.

[ry]

I see that @keroxp has a test in #4284 - do we have a smaller example we can integrate into our test suite? This PR does not add any tests.

[jsouto18]

@ry I’ll add some tests ofc. I just wanted to know what you guys thought about this solution first.

[bartlomieju]

Another test case (in std/) can be found in this PR: #3679

[ry]

We should get a test that involves only TCP code (not the http server) and just does listen, accept, read.

@jsouto18 Thanks for the patch - but we want to look into this in more detail.

[bartlomieju]

@jsouto18 can you incorporate one of tests mentioned above?

[jsouto18]

@bartlomieju I've implemented #4284 example as a test. I use a new timeout arg to test if the process is hanging.

[bartlomieju]

@bartlomieju I've implemented #4284 example as a test. I use a new timeout arg to test if the process is hanging.

That's a bit too complex; can you revert those changes and used this boiled down reproduction (put it in cli/js/tests/net_test.ts):

unitTest(
  {
    perms: { net: true }
  },
  async function netHangsOnClose() {
    let acceptedConn: Deno.Conn;
    const resolvable = createResolvable();

    async function iteratorReq(listener: Deno.Listener): Promise<void> {
      const p = new Uint8Array(10);
      const conn = await listener.accept();
      acceptedConn = conn;
      
      while (true) {
        const nread = await conn.read(p);
        console.log("read", nread, "bytes");
        if (nread === Deno.EOF) {
          break;
        }
        const nwritten = await conn.write(new Uint8Array([1, 2, 3]));
        console.log("written", nwritten, "bytes");
      }

      resolvable.resolve();
    }

    const addr = { hostname: "127.0.0.1", port: 4500 };
    const listener = Deno.listen(addr);
    iteratorReq(listener);
    const conn = await Deno.connect(addr);
    await conn.write(new Uint8Array([1, 2, 3, 4]));
    const buf = new Uint8Array(10);
    const nread = await conn.read(buf);
    console.log("main read", nread);
    console.log("accepted", acceptedConn!);
    acceptedConn!.close();
    listener.close();
    await resolvable;
  }
);

It does hang on master for me

[jsouto18]

@bartlomieju the changes are reverted and I've added the new test. 😃
The reason for the timeout implementation was to make sure an error was thrown instead of having the test suite hang indefinitely.

[ry]

@jsouto18 Thank you for this patch and thank you @bartlomieju for the succinct test.

I think there might be a deeper design issue here - but the solution isn't clear to me now - maybe we should be tracking in a more general way which ops operate on which resources. This makes me hesitate on this patch, because it introduces infrastructure for only stream resources.

That said, it fixes bugs and we have a new test that we can refactor around, so I will land it now.