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

Unbound: let the logger wait for the pipe a bit #6331

Merged
merged 1 commit into from Feb 16, 2023

Conversation

kulikov-a
Copy link
Member

Hi!
ref. https://forum.opnsense.org/index.php?topic=32494.0
looks like logger db without index initializes so much faster that pipe may not be ready yet due to json loading?
thanks!

@AdSchellevis
Copy link
Member

Hi Alex, am I right in assuming the reader (the other end) is to fast in trying to access the pipe and doesn't re-try? I'm not sure the actual issue in the dnsbl_module.py script in that case.

@kulikov-a
Copy link
Member Author

Hi Ad! )
I agree that adding a second poke to the logger.py is the most correct solution. it just seems to me that creating a pipe before json loading does not spoil anything and never failed during testing (before fixing the db speed, creating\loading the index in logger.py gave the dnsbl_module.py time to create the pipe?).
i'll try to add a retry on OSError exception in logger.py

@kulikov-a kulikov-a changed the title Unbound: create logger pipe earlier Unbound: let the logger wait for the pipe a bit Feb 16, 2023
@AdSchellevis
Copy link
Member

I'm not worried about changing the order of setup, just want to avoid masquerading the real issue. As we're currently refactoring some code in the python unbound module, I rather keep the order as is to avoid it being set back after merge anyway :)

@kulikov-a
Copy link
Member Author

@AdSchellevis got it ) done
it just opening pipe on second try now

@AdSchellevis
Copy link
Member

@kulikov-a looks good, maybe increase the wait to 1 second so it has a bit more wiggle room. the module caches anyway in the meantime

@kulikov-a
Copy link
Member Author

done ;)

@AdSchellevis
Copy link
Member

@kulikov-a looks good, thanks!

@AdSchellevis AdSchellevis merged commit 7ebe361 into opnsense:master Feb 16, 2023
@kulikov-a kulikov-a deleted the patch-41 branch February 16, 2023 14:24
fichtner pushed a commit that referenced this pull request Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants