pool: fix spurious send_event timeout error

[DanConwayDev]

start to listen for OK msg immediately before event is sent

this prevents the OK msg from being recieved
before we start listening for it

this bug was introduced in v0.28 and discovered when upgrading
v0.27 ~> v0.29 in ngit (https://https://gitworkshop.dev/ngit)
as it caused many intergration tests to fail

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

• I followed the contribution guidelines
• I ran just precommit or just check before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[yukibtc]

Maybe is enough to move the let mut notifications = self.internal_notification_sender.subscribe(); outside the time::timeout, before the self.batch_msg(msgs, opts).await?;. Should be cleaner as solution.

What do you think?

[DanConwayDev]

That's much more elegant! I've force pushed your suggestion to this branch.

[yukibtc]

ACK 7362f7c

[yukibtc]

Just published nostr-relay-pool v0.29.1

Make sure to bump it in your Cargo.lock with cargo update -p nostr-relay-pool

[DanConwayDev]

Thanks.
I am still getting frequent, but intermittent, errors on many integration tests after upgrading from rust-nostr v0.27 ~> v0.29.

They seem to be failing at points which use Relay::get_events_of.
Maybe there is a similar problem as the handler is created after the subscription is initiated in get_events_of_with_callback?

Also, possibly not related but clippy is warning about

large future with a size of 21016 bytes
for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_futures
`#[warn(clippy::large_futures)]` implied by `#[warn(clippy::pedantic)]`

for relay.connect and client.connect_relay post upgrade.

[yukibtc]

Can you point me where are the issues in the ngit code? I'll try to take a look

[yukibtc]

I found the get_events_of. Maybe the timeout is too low (500 ms) also if the relay is on the same machine. What error do you receive?

[DanConwayDev]

Thanks :-)

rust-nostr v0.29 upgrade branch

running when running cargo test, a non-consistent set of integration tests will fail with the same error:

Timeout Error: Expected EOF (End of File) but got "searching for profile and relay updates...`\r``\n`" (after waiting 2000 ms)

The first thing the tool does with a fresh config (integration test conditions) is to fetch the users metadata and relays. this is where it intermittently fails to receive the events sent by the relay. The test times out before the end of the get_events_of timeout.

You highlighted the function where this occurs.
https://github.com/DanConwayDev/ngit-cli/blob/3c534c24e5ff96d8dcd88ed419ae7d7d07e96f92/src/client.rs#L243

[DanConwayDev]

the clippy errors:

large future with a size of 21016 bytes
for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_futures
`#[warn(clippy::large_futures)]` implied by `#[warn(clippy::pedantic)]`

appear on these two lines:
https://github.com/DanConwayDev/ngit-cli/blob/3c534c24e5ff96d8dcd88ed419ae7d7d07e96f92/src/client.rs#L127
https://github.com/DanConwayDev/ngit-cli/blob/3c534c24e5ff96d8dcd88ed419ae7d7d07e96f92/src/client.rs#L249

[yukibtc]

I tried to take a look but the most of time I receive this:

X has been running for over 60 seconds

Always in the tests/list.rs

---

Regarding the warning: maybe is related to the size of InternalRelay::try_connect or it's a false positive. Not sure yet, I never saw this warning before. From the clippy book:

Note that clippy::pedantic contains some very aggressive lints prone to false positives.

https://doc.rust-lang.org/stable/clippy/configuration.html?highlight=clippy%3A%3Apedantic#allowingdenying-lints

I'll try to re-write the try_connect method for better readability and should solve also that warning.

[DanConwayDev]

Try running cargo test --test init a few times until one of the test fail.

the list tests are probably failing for the same reason but in a 'prep' phase which means they dont fail gracefully and cause the test to never end.

The main branch passes CI:
https://github.com/DanConwayDev/ngit-cli/actions/runs/8392897271/job/22987395219

[yukibtc]

Ok, yes, I received that error.

Bumping nostr-sdk to commit d0c5f15 solves the issue. I'm going to release a new patch for nostr-relay-pool

[yukibtc]

Released nostr-relay-pool v0.29.2

[DanConwayDev]

There were a few minor behavioral changes which were breaking other tests. I updated the test and they are all now passing, so I have merged the update to v0.29.

Thanks for your assistance.