Skip to content

PR for Issue 1188 (persistent timeout in call home scenarios)#64

Merged
michalvasko merged 4 commits intoCESNET:develfrom
frimpler:persistent_timeout_fix_pr
Aug 6, 2018
Merged

PR for Issue 1188 (persistent timeout in call home scenarios)#64
michalvasko merged 4 commits intoCESNET:develfrom
frimpler:persistent_timeout_fix_pr

Conversation

@frimpler
Copy link
Copy Markdown
Contributor

Hi Michal,

I interpreted the discussion reg. sysrepo/sysrepo#1188 in that way that you are fine with the proposed solution in general and want me to provide the fix for the problem stated there. I have also added the improvements for the sockets and store now the pending ones, in order to avoid numbers of half opened sockets. And, I didn't see the IPv6 problem any more when using AF_UNSPEC.

Please have a look at it and if you don't see bigger issues, please pull it in. (Btw.: I'll not be able to work on it in the next days, if you have only minor complains :-))

Regards,
Frank

Comment thread src/session_client.c Outdated
}
/* non-blocking connect! */
if (connect(sock, res->ai_addr, res->ai_addrlen) < 0) {
if (errno != EINPROGRESS) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a problem here for the case when getaddrinfo() returns multiple resolutions and we can connect successfully to the first resolution but fail to connect to the second resolution.

Because the socket is now non-blocking, for the first resolution, connect() will return a negative value with errno set to EINPROGRESS, indicating that the connection attempt is proceeding but connect() wasn't allowed to block. We'll then proceed back to the top of the loop (line 938) and try the second resolution. If the second resolution fails, we've already lost the value for sock from the first resolution (whose connection was in progress), and this function will incorrectly return a failure to connect, even though the first connection attempt might have succeeded.

@frimpler
Copy link
Copy Markdown
Contributor Author

Hi Michal,

please have a look at the new commit. I mentioned IPv6 problems I had with my first implementations but they are solved now. The reason for them was, that a non-blocking connect behaves quite different and one has to check the usability of the socket afterwards "by hand" (check for socket errors). In the first implementations to this PR, this problem was hidden, because I forgot a break at the end of the loop. This caused of course, what Andrew has complained - but it wasn't really the intention to stay in the loop with a valid resource.

Regards,
Frank

@frimpler
Copy link
Copy Markdown
Contributor Author

@michalvasko
Travis seem to have trouble to start. Any suggestion what can be done?

@michalvasko
Copy link
Copy Markdown
Member

Hi Frank,
sorry, I was away. I briefly tested it and it seems to work. However, there seems to be a leftover unused variable i in session_client.c:908 so please remove it. Afterwards, I think it can be merged.

Regards,
Michal

@michalvasko
Copy link
Copy Markdown
Member

Hi Frank,
okay, seems good. Thanks!

Regards,
Michal

@michalvasko michalvasko merged commit 2b5d7a1 into CESNET:devel Aug 6, 2018
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.

3 participants