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

Add integration tests for transports #2194

Closed
9 of 11 tasks
MarcoPolo opened this issue Mar 16, 2023 · 4 comments
Closed
9 of 11 tasks

Add integration tests for transports #2194

MarcoPolo opened this issue Mar 16, 2023 · 4 comments
Assignees

Comments

@MarcoPolo
Copy link
Collaborator

MarcoPolo commented Mar 16, 2023

We should have some integration tests for transports in general to:

  1. Codify what well-behaved transports need to do.
  2. Make it easier to write new transports.

The tests should at least exercise:

@MarcoPolo MarcoPolo self-assigned this Mar 16, 2023
@marten-seemann
Copy link
Contributor

marten-seemann commented Mar 17, 2023

  • One stream more data (e.g. 1MiB).

Ideally, more than 1 MB. 1 MB still might fit in the initial flow control window, depending on the transport. 20 MB sounds like a safer choice, while still being reasonably fast.

  • Stream resets.

That's 2 tests: The sender resetting the write side and the receiver resetting the read side.

Not a WIP any more after today's work: #2200.
Ideally, the transport tests would live in the same package, so we only have to define the address slice (maybe refactored to be a slice of libp2p.Options) once:

var addrs = []ma.Multiaddr{
ma.StringCast("/ip4/127.0.0.1/tcp/0"),
ma.StringCast("/ip4/127.0.0.1/tcp/0/ws"),
ma.StringCast("/ip4/127.0.0.1/udp/0/quic"),
ma.StringCast("/ip4/127.0.0.1/udp/0/quic-v1"),
ma.StringCast("/ip4/127.0.0.1/udp/0/quic-v1/webtransport"),
}

  • deadline on streams

I'm particularly worried about WebSocket here:

The documentation says:

When a deadline is hit, the connection will be closed. This is different from most net.Conn implementations where only the reading/writing goroutines are interrupted but the connection is kept alive.

And as far as I can tell, we're doing nothing to work around this. If that's the case, this doesn't need to block the PR. We can disable the test for WebSocket and create an issue to fix this in a follow-up PR.


Adding to the list in the original post (feel free to copy-paste):

  • Multiple streams (concurrently). The sum of the stream data transferred should exceed 20 MB, for the same reason as above.
  • Many streams, more than the muxer's stream limit (N = 1000?). Or alternatively, setting a lower rcmgr limit on the receiver side. The client would need to back off on stream resets then (wait for 50ms, then try again). Eventually, both server and client should have handled exactly N streams.

The second test maybe doesn't need to be in the first iteration, if it's adds too much complexity.

@BigLep
Copy link
Contributor

BigLep commented Jun 19, 2023

Is the next step here reviews on #2286 and #2295. I see there hasn't been progress for a couple of weeks. I'd love to see these get over the line so we can close this issue.

@MarcoPolo
Copy link
Collaborator Author

Yes, I'm waiting for reviews there and open questions. Right now this isn't blocking anything and there are other things this week that have a higher priority (e.g. internal perf reviews), so I probably won't get to this this week.

@MarcoPolo
Copy link
Collaborator Author

This is done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants