Fix unwrap or default to avoid panics

[seb-posedio]

Recently I ran into multiple panics due to unwrap not being able to
return a value.

In order to fix the exception=PanicException('called Option::unwrap()on aNone value')>, all unwraps have been transformed to either unwrap
or default to return either None or the value itself, which is
considered a best practice.

[Yiling-J]

@seb-posedio Thanks for the PR, I agree that we should avoid using unwrap whenever possible. However, one concern I have is that in the places where unwrap is currently used (for example, entry.policy_list_index.unwrap()), a panic would actually indicate a bug. These cases should never panic, and if they do, I want to know so I can fix the issue.

If we replace unwrap with an if let Some(...) without an else branch, we lose that signal and the error becomes silent. At the very least, we should log the error so it’s visible. I don’t write Rust a lot, so there may be better approaches here, feel free to suggest alternatives.

BTW did you find out where your panic came from? That already suggests there’s a bug that I need to fix.

[seb-posedio]

Hey @Yiling-J, thanks for reviewing & maintaining this library!

@seb-posedio Thanks for the PR, I agree that we should avoid using unwrap whenever possible. However, one concern I have is that in the places where unwrap is currently used (for example, entry.policy_list_index.unwrap()), a panic would actually indicate a bug. These cases should never panic, and if they do, I want to know so I can fix the issue.

I agree, but as this is a cache, I wouldnt be too concerned about a cache miss. If its a database, that would be a different story as this would really be painful to have potential dataloss. But yes, an indication that something is faulty would be very nice.

If we replace unwrap with an if let Some(...) without an else branch, we lose that signal and the error becomes silent. At the very least, we should log the error so it’s visible. I don’t write Rust a lot, so there may be better approaches here, feel free to suggest alternatives.

Instead of silently dropping, I propagate the Error as Exception, so upstream can deal with it & do proper error handling.

BTW did you find out where your panic came from? That already suggests there’s a bug that I need to fix.

I am running a DNSBL Server, previously written in Python and the requests have been increased since the beginning, so for short & mid-term I had to introduce a cache to avoid service degragation because the database is not built to handle this load. Longterm its already planned to switch to a Rustlang alternative.

After running the server for hours & millions of queries, it starts to hit - which is likely to happen as this runs in an async environment

dnsbl-1  | thread '<unnamed>' panicked at src/tlfu.rs:372:47:
dnsbl-1  | called `Option::unwrap()` on a `None` value
dnsbl-1  | note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
dnsbl-1  | Task exception was never retrieved
dnsbl-1  | future: <Task finished name='Task-1265721' coro=<DNSServerProtocol.handle_query() done, defined at /app/code/service.py:437> exception=PanicException('called `Option::unwrap()` on a `None` value')>
dnsbl-1  | Traceback (most recent call last):
dnsbl-1  |   File "/app/code/service.py", line 440, in handle_query
dnsbl-1  |     reply = await self.resolver.resolve(data, addr)
dnsbl-1  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
dnsbl-1  |   File "/app/code/service.py", line 400, in resolve
dnsbl-1  |     CACHED_RECENT.set(int(ip), cache_data, timedelta(hours=4))
dnsbl-1  |   File "/app/venv/lib/python3.12/site-packages/theine/theine.py", line 548, in set
dnsbl-1  |     self._write_buffer.add(kh, ttl_ns)
dnsbl-1  |   File "/app/venv/lib/python3.12/site-packages/theine/write_buffer.py", line 36, in add
dnsbl-1  |     evicted = self.send_to_core(wb)
dnsbl-1  |               ^^^^^^^^^^^^^^^^^^^^^
dnsbl-1  |   File "/app/venv/lib/python3.12/site-packages/theine/theine.py", line 506, in _drain_write
dnsbl-1  |     return self.core.set(entries)
dnsbl-1  |            ^^^^^^^^^^^^^^^^^^^^^^
dnsbl-1  | pyo3_runtime.PanicException: called `Option::unwrap()` on a `None` value

[Yiling-J]

In core.rs, it looks like the errors are still invisible to the user? We should at least log the error, and since Theine is a Python package, it might be good to use pyo3-log so we follow the logging configuration on the Python side.