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: switch to async iterators #81

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

vasco-santos
Copy link
Member

BREAKING CHANGE: Switch to using async/await and async iterators. The transport and connection interfaces have changed.

BREAKING CHANGE: Switch to using async/await and async iterators. The transport and connection interfaces have changed.
src/socket.js Outdated Show resolved Hide resolved
src/adapter.js Outdated Show resolved Hide resolved
@vasco-santos vasco-santos force-pushed the refactor/async branch 2 times, most recently from 4d249c5 to 585ea74 Compare October 8, 2019 19:50
@vasco-santos vasco-santos marked this pull request as ready for review October 8, 2019 19:50
@vasco-santos
Copy link
Member Author

@jacobheun similarly to what we experienced in webrtc, there is a test failing here from interface-transport: abort while reading throws AbortError. The next test also fails, but it is because of side effects created from the test before.

So, I believe that there is a bug in alanshaw/stream-to-it/ using toIterable. Since we did not support abortables before, should we skip the test for now and create an issue? I think there is no need in blocking this PR with the abortable feature.

@daviddias
Copy link
Member

So, I believe that there is a bug in alanshaw/stream-to-it/ using toIterable. Since we did not support abortables before, should we skip the test for now and create an issue? I think there is no need in blocking this PR with the abortable feature.

This seems to be the type of error that will haunt us for life (been there, had to open many cans of worms).

To move forward, there should be a reproducible test case (or test cases) that show this behavior on the stream-to-it module and a respective comment (and issue describing the situation) on the transports (this and WebRTC) that that test needs to pass in order for the tests here to pass.

@vasco-santos
Copy link
Member Author

I created some tests in stream-to-it using the abort controller, but I can get it to work there 🤔

vasco-santos/stream-to-it@872e44b

In this case here, after the abort is performed, the dialer connection receives the close event. However, it-pipe does not throw an error like in libp2p-tcp/ libp2p-websockets

@daviddias
Copy link
Member

However, it-pipe does not throw an error like in libp2p-tcp/ libp2p-websockets

Consider creating a test with libp2p-tcp/websockets in stream-to-it to ensure that error gets caught.

@vasco-santos
Copy link
Member Author

vasco-santos commented Oct 9, 2019

With libp2p-websockets we do not use stream-to-it as we do not need the conversion. That's why I am (or was 🤔) suspecting on an issue within the module. But doesn't seem to be the case

In libp2p-tcp it uses stream-to-it and the error is caught, otherwise it would not pass the interface-transport tests

@jacobheun
Copy link

I'm looking into it, it should work. I think there is just some order of operations issue ending the connections that's causing them to stay open.

@jacobheun
Copy link

There may be some issues with utp-native. Oddly, the outbound connection emits the close event right after sending data, but the socket properties indicate that it's actually readable and writable and it thinks it's destroyed.

@jacobheun
Copy link

jacobheun commented Oct 9, 2019

So, I believe the problem is that utp-native is overriding destroy on the readable-stream, https://github.com/mafintosh/utp-native/blob/v2.1.4/lib/connection.js#L206, it should be using _destroy per nodejs guidelines. Since Stream is not performing destroy properly, we never get the propagation to the iterators.

I did a quick hack locally of utp-native to use the following and things are passing for me locally. (These changes aren't verified against the rest of the utp-native code)

Connection.prototype._destroy = function (err, cb) {
  if (this._needsConnect) {
    process.nextTick(onbindingclose, this)
    return cb(err)
  }
  binding.utp_napi_connection_close(this._handle)
  cb(err)
}

@jacobheun jacobheun added the status/blocked Unable to be worked further until needs are met label Jun 25, 2020
@vasco-santos vasco-santos mentioned this pull request Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/blocked Unable to be worked further until needs are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants