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

Move wait at network startup #338

Merged
merged 1 commit into from
Sep 6, 2023
Merged

Move wait at network startup #338

merged 1 commit into from
Sep 6, 2023

Conversation

zeegomo
Copy link
Contributor

@zeegomo zeegomo commented Aug 29, 2023

We now wait after the call to 'subscribe' to give the network the time to register peers in the mesh before starting to publish messages

Should fix occurrences of #329 in testing for the short term

Copy link
Collaborator

@danielSanchezQ danielSanchezQ left a comment

Choose a reason for hiding this comment

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

I think this works for now. But I think we should have some way of asking the service if it is ready for receiving messages (if it is actually viable) at some point.

@zeegomo
Copy link
Contributor Author

zeegomo commented Aug 29, 2023

See my other comment in #329 (comment), I don't think it's easy to design a general ready-check

@danielSanchezQ
Copy link
Collaborator

Oh, I think I misunderstood the problem actually. I was thinking it was a startup issue in where we tried to use the network service before it was available. Never mind.

@zeegomo
Copy link
Contributor Author

zeegomo commented Aug 29, 2023

Oh, I think I misunderstood the problem actually. I was thinking it was a startup issue in where we tried to use the network service before it was available. Never mind.

In a way it is, for this specific problem, but the issue is that new services can subscribe to new topics at any moment, so being ready is first not a global concept and secondly not static in time. It could be ready now and not ready in the future and also at the same time ready for a service and not ready for another service

@danielSanchezQ
Copy link
Collaborator

Oh, I think I misunderstood the problem actually. I was thinking it was a startup issue in where we tried to use the network service before it was available. Never mind.

In a way it is, for this specific problem, but the issue is that new services can subscribe to new topics at any moment, so being ready is first not a global concept and secondly not static in time. It could be ready not and not ready in the future and also at the same time ready for a service and not ready for another service

Definetely, makes sense.

Copy link
Contributor

@youngjoon-lee youngjoon-lee left a comment

Choose a reason for hiding this comment

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

Ah, I didn't notice that there has been a sleep for the similar purpose against waku. Thank you

@youngjoon-lee
Copy link
Contributor

CI failure will be resolved once we merge #373 and #372

     Running src/tests/mixnet.rs (target/debug/deps/test_mixnet-041a6c46c89fa0a8)

running 1 test
test mixnet ... FAILED

failures:

---- mixnet stdout ----
thread 'mixnet' panicked at 'timeout: the function call took 5000 ms. Max time 5000 ms', tests/src/tests/mixnet.rs:16:1

We now wait after the call to 'subscribe' to give the network
the time to register peers in the mesh before starting to
publish messages
@zeegomo zeegomo merged commit e50ef09 into mixnet Sep 6, 2023
11 checks passed
youngjoon-lee added a commit that referenced this pull request Sep 14, 2023
* Add `mixnode` and `mixnet-client` crate (#302)

* Add `mixnode` binary (#317)

* Integrate mixnet with libp2p network backend (#318)

* Fix #312: proper delays (#321)

* proper delays

* add missing duration param

* tiny fix: compilation error caused by `rand` 0.8 -> 0.7

* use `get_available_port()` for mixnet integration tests (#333)

* add missing comments

* Overwatch mixnet node (#339)

* Add mixnet service and overwatch app

* remove #[tokio::main]

---------

Co-authored-by: Youngjoon Lee <taxihighway@gmail.com>

* fix tests for the overwatch mixnode (#342)

* fix panic when corner case happen in RandomDelayIter (#335)

* Use `log` service for `mixnode` bin (#341)

* Use `wire` for MixnetMessage in libp2p (#347)

* Prevent tmixnet tests from running forever (#363)

* Use random delay when sending msgs to mixnet (#362)

* fix a minor compilation error caused by the latest master

* Fix run output fd (#343)

* add a connection pool

* Exp backoff (#332)

* move mixnet listening into separate task

* add exponential retry for insufficient peers in libp2p

* fix logging

* Fix MutexGuard across await (#373)

* Fix MutexGuard across await

Holding a MutexGuard across an await point is not a good idea.
Removing that solves the issues we had with the mixnet test

* Make mixnode handle bodies coming from the same source concurrently (#372)

---------

Co-authored-by: Youngjoon Lee <taxihighway@gmail.com>

* Move wait at network startup (#338)

We now wait after the call to 'subscribe' to give the network
the time to register peers in the mesh before starting to
publish messages

* Remove unused functions from mixnet connpool (#374)

* Mixnet benchmark (#375)

* merge fixes

* add `connection_pool_size` field to `config.yaml`

* Simplify mixnet topology (#393)

* Simplify bytes and duration range ser/de (#394)

* optimize bytes serde and duration serde

---------

Co-authored-by: Al Liu <scygliu1@gmail.com>
Co-authored-by: Daniel Sanchez <sanchez.quiros.daniel@gmail.com>
Co-authored-by: Giacomo Pasini <Zeegomo@users.noreply.github.com>
@jakubgs jakubgs deleted the remove-wait branch February 15, 2024 09:35
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

Successfully merging this pull request may close these issues.

vote_stream is sometimes closed before the tally is met
3 participants