print tracebacks and line numbers for Lua runtime errors

[mattwigway]

Issue

This fixes #6563, and provides tracebacks and line number for Lua runtime errors. It does this by replacing all sol::functions with sol::protected_function and checking for errors after a Lua function is called. If one is detected, a traceback is printed to stderr, and an exception is thrown which causes OSRM to exit (as it did before when runtime errors were encountered).

Before:

$ osrm-extract -p ../profiles/bicycle_lts.lua bicycle_lts/bicycle_lts.osm.pbf
[lots of intermediate output]
libc++abi: terminating with uncaught exception of type sol::error: lua: error: attempt to compare two nil values

After:

[lots of intermediate output]
attempt to compare two nil values
stack traceback:
	[C]: in function 'math.max'
	...bc/git/vmt-mode-shift-study/src/routing/profiles/lts.lua:128: in function 'lts.lts_for_way'
	../profiles/bicycle_lts.lua:515: in function 'process_way'
attempt to compare two nil values
stack traceback:
	[C]: in function 'math.max'
	...bc/git/vmt-mode-shift-study/src/routing/profiles/lts.lua:128: in function 'lts.lts_for_way'
	../profiles/bicycle_lts.lua:515: in function 'process_way'
attempt to compare two nil values
stack traceback:
	[C]: in function 'math.max'
	...bc/git/vmt-mode-shift-study/src/routing/profiles/lts.lua:128: in function 'lts.lts_for_way'
	../profiles/bicycle_lts.lua:515: in function 'process_way'
attempt to compare two nil values
stack traceback:
	[C]: in function 'math.max'
	...bc/git/vmt-mode-shift-study/src/routing/profiles/lts.lua:128: in function 'lts.lts_for_way'
	../profiles/bicycle_lts.lua:515: in function 'process_way'
libc++abi: terminating with uncaught exception of type osrm::util::exception: Lua error (see stderr for traceback)

I believe the error message is printed multiple times because the error is getting hit in multiple threads before the exception kills the entire process. My understanding is that a single exception should kill the entire process, not just the thread, but I'm new to C++ so if that's incorrect changes will be required to ensure runtime errors actually terminate extraction.

Performance impact is negligible/nonexistent. With my small test PBF file, it took 3.511s to build and partition with this patch and 3.563 without.

I ran the format.sh script as requested and it made some format changes elsewhere in the files. If this is an issue I can go back and manually revert those changes. It doesn't seem to pollute the diff too much. went ahead and reverted these for ease of review.

Tasklist

I'm new to C++ and contributing to OSRM, so there's a few things I'm not completely sure of and would love feedback on.

• Make sure that errors in any Lua thread will bring the entire extraction process to a halt, not just that thread.
• I don't know what source_function and GetStringListFromFunction do. I changed these to have Lua error handling but it I'm not sure how to test them.
• Most places where profile functions are called there's a switch statement based on version; I define the result variable that I use to check for the error outside the statement. If the profile API version is increased, and the switch statements are not updated appropriately so that the lua function is always called, it's possible that the code would check for an error using an uninitialized result. I know C++ does weird things in this case - is it desirable to add an explicit check to each function to ensure the result has actually been initialized before checking if it is valid?
• CHANGELOG.md entry (How to write a changelog entry)
• update relevant Wiki pages
• add tests (see testing documentation)

Requirements / Relations

N/A

[SiarheiFedartsou]

Most places where profile functions are called there's a switch statement based on version; I define the result variable that I use to check for the error outside the statement. If the profile API version is increased, and the switch statements are not updated appropriately so that the lua function is always called, it's possible that the code would check for an error using an uninitialized result. I know C++ does weird things in this case - is it desirable to add an explicit check to each function to ensure the result has actually been initialized before checking if it is valid?

Not sure I fully get it, but we 100% control profile API versions, i.e. we won't have any bump of profile API version until we will do that on our own, so I think we can just go with this risk that in the future we may forget to update those switches, but I hope in this case some compiler warning or sanitizer check should catch it. Btw why we have those switches is just because we have to be backward compatible with all old versions of Lua profiles.

I don't know what source_function and GetStringListFromFunction do. I changed these to have Lua error handling but it I'm not sure how to test them.

As I can see both GetStringListFromFunction and source_function are just to support VERY old version 1 of Lua API, tbh it is hard to say without doing some archeology in old commits what exactly it is supposed to do, but can we may be workaround it somehow? E.g. I don't think we really need to support this feature with tracebacks for so old version of API - I think we can just agree that we support it for only the recent version(s) of Lua API.

Let me please know if you need more help here :)

[mattwigway]

@SiarheiFedartsou I think this is ready to go! I added unit tests, backed out the changes for those functions we weren't sure about, and confirmed that an error on any lua thread shuts OSRM down entirely. I don't see any wiki pages that would be affected by this change, so haven't made any updates there.

[SiarheiFedartsou]

Thanks @mattwigway !
Can you please rebase/merge this PR with master? It contains fixes for CI which was not working before...

[SiarheiFedartsou]

@mattwigway
and you should reformat code here. It can be done with ./scripts/format.sh script, make sure to use clang-format-10 please(and let me know in case of any problems please)

[mattwigway]

Ah, clang-format-10 is the piece I was missing before. Rebased and formatted.

[SiarheiFedartsou]

@mattwigway you are probably missing #include <fstream> - that’s why CI is red. Please fix when you have a time… Or let me know and I will do it on my own in the nearest days.

[mattwigway]

Oops, I meant to delete the function that uses fstream as it's from an earlier way I tried to specify the test. Will fix.

[mattwigway]

@SiarheiFedartsou I think this should be fixed now.

[mattwigway]

I'm not sure what the memory leak test is flagging. I'll investigate later this week.

[SiarheiFedartsou]

I'm not sure what the memory leak test is flagging. I'll investigate later this week.

Yeah, would be nice, usually it does not have false positives, but taking into account use case(it happens only on error and we exit program anyway) I think we can just suppress it if we won’t find a reason or it will be too complicated to fix. Also I was going to look on my own, but I think I will be able to do that only on the next week in the best case.

[mattwigway]

I think what's going on here is that when process_way crashes and the traceback is printed, the way that it's currently processing never gets freed. This would have happened before, and the only reason it's showing up now is that there is now a test of bad code fed to the extractor. Normally the exception shuts down the whole OSRM process so it doesn't matter as you say, but in the test we catch the exception to make sure the exception handling worked. So I think suppressing the warning is probably appropriate, as it won't be an issue in production.

[SiarheiFedartsou]

@mattwigway then I think you can configure it somewhere here

osrm-backend/scripts/ci/leaksanitizer.conf

Line 8 in 192d077

leak:libtbb.so

AFAIR this file follows syntax described in https://clang.llvm.org/docs/SanitizerSpecialCaseList.html, so you can suppress this specific issue.

[mattwigway]

@SiarheiFedartsou looks like that change fixed it. I rebased from master, LMK if anything else is needed.

[SiarheiFedartsou]

@mattwigway many thanks for this contribution and I will be happy to review any other PRs from you 👍