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 restart of poll in read_tmo #2026

Closed

Conversation

hermunn
Copy link
Member

@hermunn hermunn commented Aug 1, 2016

Add check for negative return value from poll in read_tmo. A negative
return value can happen if a signal interupts poll(), and then the
correct behavior is to call poll() again.

Add check for negative return value from poll in read_tmo. A negative
return value can happen if a signal interupts poll(), and then the
correct behavior is to call poll() again.
@bsdphk
Copy link
Contributor

bsdphk commented Aug 8, 2016

So my worry about this is if we get into a unkillable process situation.

Have you pondered/looked at that ?

@hermunn
Copy link
Member Author

hermunn commented Aug 8, 2016

I did not like that a signal would send us straight into a read that may or may not block (depending on the fd), and this seems to be undesired, as we can lose the timeout behavior. But, as you say, restarting the poll might pose other problems. I need to have a closer look at the callers of read_tmo.

@hermunn
Copy link
Member Author

hermunn commented Aug 8, 2016

The read_tmo seems to only be called from VCLI_ReadResult, but the latter can be called both on a (nonblocking) TCP socket (in varnishadm), or on a pipe between the manager and worker process.

When read_tmo returns before getting the full reply, VCLI_ReadResult will return CLIS_COMMS. If VCLI_ReadResult was called from mgt_cli.c, then the manager process will terminate the worker process, while varnishadm will simply give up.

If we change continue; to break;, we will not run into the potentially blocking read, but return immediately.

@bsdphk, would you prefer a break; instead of continue;?

@bsdphk bsdphk closed this in 107c4a2 Oct 24, 2016
hermunn pushed a commit that referenced this pull request Oct 25, 2016
Spotted & prodded by: 	Hermunn
Closes: #2026
@hermunn
Copy link
Member Author

hermunn commented Oct 25, 2016

Backport review: bsdphk's fix backported to 4.1 as c330106.

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