CARTAvis / carta-backend

Source code repository for the backend component of CARTA, a new visualization tool designed for the ALMA, the VLA and the SKA pathfinders.
https://cartavis.github.io/
GNU General Public License v3.0
22 stars 11 forks source link

Improve the code style in HTTP server #1324

Closed markccchiang closed 1 year ago

markccchiang commented 1 year ago

Description

Checklist

confluence commented 1 year ago

When I made the enum suggestion I didn't realise that C++ did not have an enum type with string values -- I'm happy to drop the enum idea and continue using strings. However, I have more specific remarks about how this code should be modified.

confluence commented 1 year ago

Looks good to me. Added @kswang1029 as a reviewer, for testing.

markccchiang commented 1 year ago

Looks like we can not register the routes using a loop, otherwise, it will fail when executing the command /carta_backend --frontend_folder /Users/path/to/carta-frontend/build --port 3002 --omp_threads 8 --debug_no_auth --no_browser --verbosity 5. I am wondering if it is related to the life circle of the handler functions. @confluence do you have any ideas?

github-actions[bot] commented 1 year ago

Code Coverage

Package Line Rate Health
src.Cache 66%
src.DataStream 52%
src.FileList 67%
src.Frame 50%
src.HttpServer 42%
src.ImageData 28%
src.ImageFitter 83%
src.ImageGenerators 43%
src.ImageStats 76%
src.Logger 38%
src.Main 52%
src.Region 18%
src.Session 30%
src.Table 52%
src.ThreadingManager 87%
src.Timer 85%
src.Util 48%
Summary 38% (7087 / 18728)
markccchiang commented 1 year ago

Looks like we can not register the routes using a loop, otherwise, it will fail when executing the command /carta_backend --frontend_folder /Users/path/to/carta-frontend/build --port 3002 --omp_threads 8 --debug_no_auth --no_browser --verbosity 5. I am wondering if it is related to the life circle of the handler functions. @confluence do you have any ideas?

I guess this may be due to the interpretation of the code during the compiling time. The argument variable in the handler function has to be global. Otherwise, the registered handler function will not be valid and fail to open the frontend. @kswang1029 could you please check again if the problem is solved? Thank you!

confluence commented 1 year ago

@markccchiang what error did you get with the previous version of the code? I can't reproduce it here (and valgrind isn't reporting any issues either). This could be a difference between compilers.

I agree that it has to do with the lifetime of the object_type variable. The lambdas are executed after the RegisterRoutes function has exited, so any objects defined in the function's scope have been destroyed. Your original object_type was a reference to a value in a vector defined in the function scope. This variable doesn't have to be global, but it does have to outlive the execution of the function. I believe that you could also have fixed this by explicitly capturing the variable by copy in all of the lambdas, but I think that the current code is fine -- using the existing map keys instead of repeating the strings is cleaner.

markccchiang commented 1 year ago

@markccchiang what error did you get with the previous version of the code? I can't reproduce it here (and valgrind isn't reporting any issues either). This could be a difference between compilers.

@confluence with my Mac compiler I got an error message when opening the frontend by the command /carta_backend --frontend_folder /Users/path/to/carta-frontend/build ... like this:

Screenshot 2023-10-31 at 13 39 44

This was first found by @kswang1029. It was also found on a specific computer when opening the CARTA v4.0 electron app (with the dev branch). Not sure if it is related to the same root cause of the problem.