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

sock: Introduce xmpp_sock_t abstraction #221

Merged
merged 1 commit into from
Jun 21, 2023
Merged

sock: Introduce xmpp_sock_t abstraction #221

merged 1 commit into from
Jun 21, 2023

Conversation

pasis
Copy link
Member

@pasis pasis commented Jun 5, 2023

This patch needs review and testing!

libstrophe uses non-blocking sockets and the connect() syscall may return before a TCP connection is established. This doesn't allow to catch all possible errors synchronously and some of the errors are handled in the event handler.

In a scenario with multiple SRV records and/or multiple IP addresses resolution, we need to repeat connection attempt on a failure.

xmpp_sock_t resolves the above problem. It keeps resolved records and addresses to repeat connect attempt asynchronously.

@sjaeckel
Copy link
Member

sjaeckel commented Jun 5, 2023

CC @nosnilmot

Copy link
Contributor

@nosnilmot nosnilmot left a comment

Choose a reason for hiding this comment

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

This is looking good and I've verified it handles failures well in the tests I've done. Just a few minor points.

src/conn.c Outdated Show resolved Hide resolved
src/conn.c Outdated Show resolved Hide resolved
src/event.c Outdated Show resolved Hide resolved
src/conn.c Outdated Show resolved Hide resolved
src/sock.c Outdated Show resolved Hide resolved
@pasis
Copy link
Member Author

pasis commented Jun 16, 2023

@nosnilmot I've resolved unsigned socket issue in all places I found. And returned user setsockopt functionality back before connect() syscall. Could you please check current patch again?

@pasis pasis requested a review from nosnilmot June 16, 2023 14:36
libstrophe uses non-blocking sockets and the connect() syscall may
return before a TCP connection is established. This doesn't allow
to catch all possible errors synchronously and some of the errors
are handled in the event handler.

In a scenario with multiple SRV records and/or multiple IP addresses
resolution, we need to repeat connection attempt on a failure.

xmpp_sock_t resolves the above problem. It keeps resolved records and
addresses to repeat connect attempt asynchronously.
@fantomfp
Copy link

fantomfp commented Jun 18, 2023

libstrophe-profanity-dev-profrc.txt
libstrophe-profanity-dev-debug.log
For the Issue #217 :

Some infos about the test environment:

sources of libstrophe: git repo (xmpp-sock branch)

libstrophe has been built in a Fedora 38 x86_64 chroot

libstrophe has been installed on the system inside a chroot (Fedora 38 x86_64)

The Fedora RPM of libstrophe was reused (dirty RPM / dirty packaging) to manage the installation in the system

profanity was installed via RPM (profanity-0.13.1-3.fc38.x86_64)

a standard user has been created and configured inside the chroot

profanity was launched as non-root

debug log is sent in attachment as proof

WORKS FOR ME.

Please merge as soon as possible :)

Some infos about the infra:

1 jabber server used as main server. 1 server as fallback.

DNS zone is configured to always redirect clients to the main server. If it doesn't answer, clients will try to connect to the secondary server.

$ dig SRV +short _xmpp-client._tcp.casperlefantom.net.
10 0 5222 jabber2.casperlefantom.net.
5 0 5222 jabber.casperlefantom.net.

For the tests, I turned off the main server (ejabberd) during few minutes. All clients got disconnected. Then, they reconnected on the fallback server.

For the /reconnect settings used during the tests, I'm adding my profrc in attachment.

XMPP account is a test account.

Copy link
Contributor

@nosnilmot nosnilmot left a comment

Choose a reason for hiding this comment

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

all my previous comments have been addressed 😄

Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

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

Thanks! 👍 very nice

Waiting on the re-review of @nosnilmot before merging. Apparently my GH page was not refreshed since yesterday ... thx @nosnilmot

Thx @fantomfp for the test report.

@sjaeckel sjaeckel merged commit 6d2864b into master Jun 21, 2023
@sjaeckel sjaeckel deleted the xmpp-sock branch June 21, 2023 12:36
@sjaeckel sjaeckel added this to the next milestone Jul 26, 2023
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.

4 participants