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

Fix index out of range for stop_times last arrival of multi-section run #704

Merged
merged 2 commits into from
Jan 18, 2025

Conversation

traines-source
Copy link
Contributor

@traines-source traines-source commented Jan 18, 2025

I stumbled upon a motis crash libc++abi: terminating due to uncaught exception of type cista::cista_exception: bucket::at: index out of range with the 06.01.2025 DELFI GTFS and this query (didn't dare to try it out on one of the public instances...):
/api/v1/stoptimes?stopId=gtfs_de%3A03159%3A33817%3A%3A8&time=2025-01-18T13%3A08%3A00.000Z&arriveBy=true&n=10

Occurs in nigiri/src/rt/frun.cc#L105 for the last stop of multi-section trips, because it would always look for a event_type::kDep.
I.e. an alternative fix with less (interface) changes would be to make get_trip_index(ev_type) in nigiri more robust, but would then still lead to wrong results for the trip_id at stops where the trip_id switches (if I understand correctly).

include/motis/tag_lookup.h Outdated Show resolved Hide resolved
@felixguendling
Copy link
Member

Thank you! 🌠

Do you think it makes sense to add a utl::verify somewhere to prevent this from happening in case the wrong event type would be supplied in the future?

@felixguendling
Copy link
Member

Hmm.. it already fired an exception. I would've hoped that this gets caught somewhere in the HTTP handler.

Co-authored-by: Felix Gündling <[email protected]>
@traines-source
Copy link
Contributor Author

traines-source commented Jan 18, 2025

Yes rather than utl::verify (which would basically have the same result, just with a nicer error message, right?) I would maybe make get_trip_index more robust (but now that the event_type param is everywhere it forces you to think about it, so it should be ok like that as well). And yes I was surprised that this took down motis entirely, not only the thread or sth.

@felixguendling
Copy link
Member

felixguendling commented Jan 18, 2025

@felixguendling felixguendling merged commit 3c361bb into motis-project:master Jan 18, 2025
11 of 12 checks passed
@felixguendling
Copy link
Member

felixguendling commented Jan 18, 2025

Ah. I guess it's because those methods in frun are marked noexcept. Maybe it would be better to just get writ of noexcept in MOTIS code completely except if it's proven to improve performance (which is rarely the case - and if it improves performance it's usually not really a big difference).

@traines-source
Copy link
Contributor Author

Ah yes. And maybe we should, because a dying motis is a bad thing :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants