Fix stealth exceptions and blocking select() in daemonize()

[vaygr]

SUMMARY

This came out of the debugging session from ansible-collections/community.general#6618 where a module must use daemonize() to work.

daemonize() was ported to module_utils.service in #21256 with a couple of issues that are not reproducible with the code from the now-deprecated service module. I couldn't find use of it alone in both ansible and community.general collection repos, so it seems it wasn't tested thoroughly afterwards.

This PR fixes a few things:

1. Padding bug on L220: local variable 'data' referenced before assignment
2. Type bugs on L220 and L251: 'bytes' object has no attribute 'encode'
3. The biggest logical one on L214 is two-fold:
   1. p.poll() should be replaced with p.poll() is None as it was in the deprecated service module since as per #subprocess.Popen.poll it can return 0, 1, 2, etc. (any return code) as well as None while the process hasn't terminated.
   2. Missing else: break that's also present in the deprecated service module: because we have a 2-process communication (parent, child) the condition on L214 is never satisfied, and the parent's select() blocks indefinitely waiting for the child on L246 (#select.select). By adding else: break we exit the loop when the process started by the child exits.

cc @bcoca

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

module_utils.service

ADDITIONAL INFORMATION

Relevant code from modules.service: service.py#L301-L335

[sivel]

We've decided to not accept these changes. As no current module uses this code, and we have no need for it in ansible-core, this should be pulled into the collection you need, updated to work as you expect, and separately we need to deprecate this and remove it from ansible/ansible.

[vaygr]

@sivel Seems I missed that the sysvinit module still uses it if daemonize option is specified. It doesn't seem it's used broadly though, based on my GitHub search.

Do you want me to deprecate that option as well or there are no plans to take care of this atm?

[sivel]

@vaygr give me a little while to validate what you just said. We may have missed the sysvinit module also.

[vaygr]

Yeah, I'm thinking there could be valid use cases for this, even though rare:

    daemonize:
        type: bool
        description:
            - Have the module daemonize as the service itself might not do so properly.
            - This is useful with badly written init scripts or daemons, which
              commonly manifests as the task hanging as it is still holding the
              tty or the service dying when the task is over as the connection
              closes the session.
        default: no

[sivel]

Ok, we did miss that module, not sure how, but thank you for bring it to my attention.

I'm going to dismiss my review, and then it'll require an actual code review from the core team, which likely won't come from me.

Errant evaluation

[vaygr]

Hey @sivel @bcoca, I was wondering if somebody had a chance to take a look at it, as it's blocking ansible-collections/community.general#6618.