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

Bug in ini library #556

Closed
mcblum opened this issue Aug 30, 2024 · 11 comments
Closed

Bug in ini library #556

mcblum opened this issue Aug 30, 2024 · 11 comments
Labels
bug Something isn't working

Comments

@mcblum
Copy link

mcblum commented Aug 30, 2024

Bashly Version

Latest Ruby Gem

Description

What I'm seeing is that sometimes values are added or deleted from the wrong section. I was banging my head against the wall trying to figure out what was going on and then I ran a test operation and asked it to delete tenant.DATA_PATH from one section but it, in fact, deleted it from another. This has caused some serious issues.

Also, sometimes the ini file is just blank altogether, which I really don't understand.

Contents of bashly.yml

No response

Reproduction Steps

Basically I'm looping through a bunch of tenant folders, reading their INI files, and sometimes updating values. The updates are frequently getting messed up.

Actual Behavior

Keys are randomly deleted from the ini file, sometimes the ini file is written blank.

Expected Behavior

That loading the INI and changing a value would work as expected

@mcblum mcblum added the bug Something isn't working label Aug 30, 2024
@DannyBen
Copy link
Owner

Without a minimal, simple way to reproduce the problem, there is not much I can do.

@mcblum
Copy link
Author

mcblum commented Aug 30, 2024

I'm working on it right now! I would guess that means this isn't something that people are reporting often?

@DannyBen
Copy link
Owner

No reports. You can also take a look at the code for the INI library, it really doesn't do much.

ini_save() {
declare -gA ini
local ini_file="$1"
local current_section=""
local has_free_keys=false
rm -f "$ini_file"
for key in $(ini_keys); do
[[ $key == *.* ]] && continue
has_free_keys=true
value="${ini[$key]}"
echo "$key = $value" >>"$ini_file"
done
[[ "${has_free_keys}" == "true" ]] && echo >>"$ini_file"
for key in $(ini_keys); do
[[ $key == *.* ]] || continue
value="${ini[$key]}"
IFS="." read -r section_name key_name <<<"$key"
if [[ "$current_section" != "$section_name" ]]; then
[[ $current_section ]] && echo >>"$ini_file"
echo "[$section_name]" >>"$ini_file"
current_section="$section_name"
fi
echo "$key_name = $value" >>"$ini_file"
done
}

@Nsomnia
Copy link

Nsomnia commented Aug 30, 2024

Do a vs line by line step debug, bang in some sleep/echo/printf commands in bashly, bashdb/similar, or whatever you might be using as libs/src with related ideas, to figure out when and why it happens?

@DannyBen
Copy link
Owner

I am not following.

If it is a bug in the INI library in bashly, it should be easily reproducible.
If the code to reproduce is it complicated, you need to uncomplicate it, and be sure it is a bug in the INI library provided with bashly, and not elsewhere in your script.

If it is inconsistent, then I suspect a race condition.

Keep in mind that the code libraries provided with bashly are provided as a basis for you to build on. It is not the primary purpose of bashly to provide you with bash snippets. That said, if there is an obvious bug with any of the provided libraries, I would like to have them fixed.

@mcblum
Copy link
Author

mcblum commented Aug 30, 2024

If it is inconsistent, then I suspect a race condition.

This is exactly what I was thinking, but I can't find where that would be happening. I'm finding it hard to reproduce because IRL there are a lot of variables, so it's a lot of stuff to fake. The general process for us is

  • loop through tenants
  • for each tenant, load their ini
  • read/write some config
  • continue on

I suspect that's where the issue might be. I also hear what you're saying about it not being a Bashly thing, and that makes sense.

@DannyBen
Copy link
Owner

DannyBen commented Aug 30, 2024

As you can see from the first line in the ini_save() function:

ini_save() {
declare -gA ini

it relies on a global variable - to set/get values.

This means that if it is called more than once at the same time, you will see issues. It was not designed for it.
See if there is any chance your code calls it with overlapping calls.

Another thing it was not necessarily designed for - and perhaps this CAN be reproduced - is to handle multiple INI files at once - even if it is one after the other. It was designed to be more of a function to let you read your own INI config file. So perhaps this is a direction where we can try and reproduce - just do a loop of read/writing - I suspect that the global variable may remain "contaminated" in some scenarios.

@mcblum
Copy link
Author

mcblum commented Aug 30, 2024

When you say with overlapping calls, do you mean within the same function run or two things running the script at the same time? I think you're absolutely correct about it being an issue with multiple ini files. I added some code to work around this by, essentially, clearing the ini array as part of a reset function, but I would guess that's where the issue is, too.

I'll hack on this to see what I can do with it to expand/improve upon it. I think maybe I'll attempt to add something like:

config_load "$FILEPATH" "$NAME"

If filepath and name are set, then it would create an array called name_ini instead of just ini and then you could perform operations on a specify an array and I wouldn't have to clear things out all the time.

@DannyBen
Copy link
Owner

DannyBen commented Aug 30, 2024

See if this helps:

Testing multiple INI files loaded in the same script causes an issue now. Here is the reproduction:

ini_load "test.ini"

for key in $(ini_keys); do
  echo "- $key = ${ini[$key]}";
done

# this is the fix.
# without it, the next piece of code will show entries from both INI files.
unset ini   

ini_load "test2.ini"

for key in $(ini_keys); do
  echo "- $key = ${ini[$key]}";
done

This will "blend" all configuration from all loaded INI files, unless you call "unset ini" as I did in the script.
Could this be the same issue you see?


Of course, you can add the "unset ini" line inside the load_ini() function.
If this is indeed your issue, we can probably safely add it to the library itself.

@DannyBen
Copy link
Owner

DannyBen commented Aug 30, 2024

In general, you should consider this:

The few bashly libraries you can add with bashly add are added to your folder by design.
The reason being, that you should treat them almost as if they are your own code.

  • If they are not to your liking - change them.
  • If you see a bug in them - fix it inside their own functions
  • If the changes/fixes are something that might be interesting to others, we can discuss updating the base libraries shipped with bashly.

In the case you discovered - it seems like adding unset ini, or providing means for specifying the name of the variable (or both) - might be of interest to others.

So - I am waiting to hear if adding unset ini inside the ini_load function seems to be solving your issue. If so, we can proceed.

@DannyBen
Copy link
Owner

Implemented in version 1.2.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants