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

improvement: get rid of subshell + exec in helper-functions.sh #2401

Merged
merged 3 commits into from Feb 8, 2022

Conversation

georglauterbach
Copy link
Member

Description

The new way of executing sha512sum should work as well as the old way
but without the clutter and possible problems the usage of subshells +
exec incurs.

Moreover, there was a misconception about array expansion. Using ""
around an expanding array (${ARRAY[@]}) is quite fine (and actually
the preffered way), not because it makes the expansion one string
(this would be ${ARRAY[*]}), but it makes sure when elements are
expanded, each element has "" around them so to speak, i.e. there is
no re-splitting of these elements.

This PR is trying to a be step in uncluttering the changedetector process to find out what is causing the resource leak described in #2348.

Type of change

  • Possible bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

The new way of executing `sha512sum` should work as well as the old way
but without the clutter and possible problems the usage of subshells +
exec incurs.

Moreover, there was a misconception about array expansion. Using `""`
around an expanding array (`${ARRAY[@]}`) is quite fine (and actually
the preffered way), not because it makes the expansion _one_ string
(this would be `${ARRAY[*]}`), but it makes sure when elements are
expanded, each element has `""` around them so to speak, i.e. there is
no re-splitting of these elements.
@georglauterbach georglauterbach added priority/high area/scripts kind/improvement Improve an existing feature, configuration file or the documentation kind/bugfix labels Feb 8, 2022
@georglauterbach georglauterbach added this to the v10.5.0 milestone Feb 8, 2022
@georglauterbach georglauterbach requested a review from a team February 8, 2022 11:43
@georglauterbach georglauterbach self-assigned this Feb 8, 2022
@georglauterbach
Copy link
Member Author

When this is merged, #2398 can possibly be closed.

casperklein
casperklein previously approved these changes Feb 8, 2022
wernerfred
wernerfred previously approved these changes Feb 8, 2022
@NorseGaud NorseGaud self-requested a review February 8, 2022 12:35
NorseGaud
NorseGaud previously approved these changes Feb 8, 2022
@georglauterbach
Copy link
Member Author

Alright, we have one failing check (which is related as well):

not ok 10 checking changedetector: can detect changes & between two containers using same config in 15732ms
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 231,
#  in test file test/mail_changedetector.bats, line 58)
#   `assert_output --partial "postfix: stopped"' failed
# 
# -- output does not contain substring --
# substring (1 lines):
#   postfix: stopped
# output (12 lines):
#   [[ TASKS ]]  2022-02-08 12:06:26 Start check-for-changes script.
#   [[  INF  ]]  2022-02-08 12:06:26 Using postmaster address postmaster@my-domain.com
#   [[  INF  ]]  2022-02-08 12:06:37 check-for-changes is ready
#   [[  INF  ]]  2022-02-08 12:06:37 Change detected
#   [ WARNING ]  Lock file /tmp/docker-mailserver/check-for-changes.sh.lock exists. Another check-for-changes.sh execution is happening. Trying again shortly...
#   [[  INF  ]]  Checking file line endings
#   [[  INF  ]]  Regenerating postfix user list
#   [[  INF  ]]  Creating user 'user1' for domain 'localhost.localdomain'
#   [[  INF  ]]  Creating user 'user2' for domain 'otherdomain.tld'
#   [[  INF  ]]  Creating user 'user3' for domain 'localhost.localdomain' with attributes 'userdb_mail=mbox:~/mail:INBOX=~/inbox'
#   [[  INF  ]]  Adding regexp alias file postfix-regexp.cf
#   [[  INF  ]]  Configuring root alias
# --
#

To me, it seems as if the restart of Postfix is about to happen because we see that the script says: Change detected. Maybe it's too slow at the moment because ealier, the exec would somehow interplay with the file lock (that is now being waited for, I guess ([ WARNING ] Lock file /tmp/docker-mailserver/check-for-changes.sh.lock exists. Another check-for-changes.sh execution is happening. Trying again shortly...)). @NorseGaud @casperklein @wernerfred @polarathene any idea? Maybe the weird exec had a bad tendency to masquerade a problem with locks in the changedetector?

@georglauterbach
Copy link
Member Author

Yes, just tested it - increasing the timeout (by a mere 10s) does the job. I think this is absolutely fine. I will push a fix, and increase the whole script sleep duration from 1s to 2s as well.

@NorseGaud
Copy link
Member

Something to consider: sleep values are dependent on a certain state. If heavy load is happening, are these sleep values valid? It's typically much better to check for some sort of state instead of relying on sleep. Especially when you find yourself increasing or decreasing sleep values.

@georglauterbach
Copy link
Member Author

Something to consider: sleep values are dependent on a certain state. If heavy load is happening, are these sleep values valid? It's typically much better to check for some sort of state instead of relying on sleep. Especially when you find yourself increasing or decreasing sleep values.

You're very right. We should consider removing sleeps as much as possible, but I have to admit, my main focus was on removing the exec :)

@georglauterbach
Copy link
Member Author

I will leave this open overnight in case @polarathene has something to add here. If you don't, by all accounts, go ahead and merge this PR @polarathene :)

Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks for the improvement and cleanup!

target/scripts/helper-functions.sh Show resolved Hide resolved
target/scripts/helper-functions.sh Show resolved Hide resolved
@polarathene polarathene merged commit ede2b23 into master Feb 8, 2022
@polarathene polarathene deleted the improvement/get-rid-of-subshell branch February 8, 2022 22:21
georglauterbach added a commit that referenced this pull request Feb 10, 2022
Due to the changes in #2401 and the chages introduced here,
`check-for-changes.sh` needs just a little bit longer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/scripts kind/improvement Improve an existing feature, configuration file or the documentation priority/high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants