Changes for JSONAPI to work with plugins - #364
Conversation
|
364, 3289 and 124 add new entries in the reviewers todo list, which is already huge. Anyway here is an audit by Claude, to facilitate reviewing. Reviewed at The gap is real and worth closing: Three things need fixing before merge. The second changes behaviour for every user who has a provider-registering plugin installed, with no action on their part. Blockers1.
|
- Moved mJsonApi to the end of RsPlugInInterfaces. - Enforced RS_PLUGIN_API_VERSION; incompatible plugins now get PLUGIN_STATUS_WRONG_API. - Added a GUI message for incompatible plugins. - Removed automatic JSON API startup based only on provider presence. - Removed FeedReader’s per-plugin startup restart. - Core now restarts JSON API once after all plugins register, only when already running. - Added mutex protection and provider snapshots for mResourceProviders.
|
fix by chatgpt 5.6
|
|
Re-reviewed at Thanks for the follow-up commit — the four actionable points from the previous round are all addressed, and addressed properly rather than papered over. I re-checked each one against the diff:
The core-owned single restart is in the right place conceptually, but as written it runs on the wrong thread in exactly the deployments it targets. That is the one blocker left, and there is a related issue in #3289's shutdown path. BlockerThe core restart runs on the JSON API server thread on Android and on webui-driven service login
restbed dispatches those handlers on the JSON API server thread itself: So on Android, where
Net effect: the JSON API is stopped and never comes back, two stacktraces in the log, and the in-flight login response most likely never delivered. The client is left talking to a dead API — and the plugin routes this PR exists to publish are still not published, since these are precisely the two cases where This is the hazard the existing /* Wrap inside RsThread::async because this call fullstop() on
* JSON API server thread.
* Calling RsThread::fullstop() from it's own thread should never
* happen and if it happens an error message is printed
* accordingly by RsThread::fullstop() */
if(!retval) RsThread::async([this](){ unProtectedRestart(); });The same pattern applies here — wrap the new restart in Related, in #3289: the restart moved from startup to shutdownFlagging it here because the two PRs land together. The per-plugin startup restart was correctly removed from mInterfaces.mJsonApi->unregisterResourceProvider(*mJsonApiProvider);
if(mInterfaces.mJsonApi->isRunning())
mInterfaces.mJsonApi->restart(true);
There is no self-join here: the Minor
What is good
This PR was not compiled during the review; the findings come from reading the sources. The blocker reproduces by logging in from the web interface against a |
|
maybe you can make it better, claude wont like then my prs |
StartupRetroShare() restarts the JSON API once after setInterfaces() so plugin resource providers get published in retroshare-service and on Android, where the server is already running by then. Those are exactly the two setups where StartupRetroShare() can be reached from the JSON API server thread: it is called by RsLoginHelper::attemptLogin() and createLocationV2(), both exposed through the API, and restbed serves its handlers on the thread that called Service::start() (no worker limit is set, so restbed defaults to running the io_context inline) -- that thread being JsonApiServer::run(). restart() calls RsThread::fullstop(). Joining our own thread makes waitWhileStopping() print an error and return without waiting, and the following RsThread::start() then fails with "attempt to start already running thread". The server ends up stopped for good, the in-flight login response is never delivered, and the plugin routes this restart exists to publish are still missing. Wrap the restart in RsThread::async, the same way the /rsJsonApi/restart handler already does for the same reason. As a side effect the RESTART_BURST_PROTECTION wait, which throttles API clients rather than the core, no longer blocks startup.
No behaviour change, three comment-level fixes from the review: - rsjsonapi.h: the ResourceProviderSet alias had been inserted between the doc comment and the functions that comment documents, so the comment read as documentation for the alias. Move the alias above it and note that getResourceProviders() now returns a snapshot by value. - rsjsonapi.h: rsJsonApi being non-null no longer implies the server is configured, since StartupRetroShare() now publishes it before startupWebServices() applies the config manager, tokens, port and binding address. Existing code uses the null-ness of that pointer as a phase marker, so say it on the declaration. - rsinit.cc: the creation block has to stay after the pre-existing connectToConfigManager() block, otherwise the GUI connects the config manager twice and reloads jsonapi.cfg twice. That ordering was load bearing and undocumented.
…read Dispatch the post-plugin JSON API restart off the API server thread
rsGlobalShutDown() stopped the JSON API almost last, after stopPlugins(). A plugin that registered a JsonApiResourceProvider deletes it in its stop(), but the running restbed service still holds the restbed::Resource objects that provider returned, and their handlers capture it. Any request served between stopPlugins() and the fullstop at the end of the function therefore dereferences freed memory. The window is not theoretical: everything in between -- UPnP teardown, the auto-proxy shutdown, all registered service threads, the RsServer tick thread and the per-peer streamers -- can take tens of seconds, and a web interface polls throughout. Move the fullstop to the top of the function. It also keeps an API client from touching the configuration after ConfigFinalSave(), and it must stay outside the wasReady branch: retroshare-service and Android start the JSON API before login, so a shutdown from that state has to stop it too. Without this, a plugin has to restart the whole JSON API from its stop() to make deleting its own provider safe, which costs a burst-protection wait and brings the server back up in the middle of teardown.
…r-364 Stop the JSON API before plugins delete their resource providers
startupWebServices() casts rsJsonApi to JsonApiServer to reuse the instance the plugin handoff created. If that cast ever fails, the previous code built a second server and overwrote the global without a word -- while every plugin still holds the pointer it was handed in setInterfaces(), now pointing at an object nobody drives. Unreachable as things stand, since rsJsonApi is only ever set to a JsonApiServer; worth one line of log rather than a silent swap. The trailing `rsJsonApi = jas;` at the end of the function repeated what the same function already did on the line above the cast, and only when it had created the server itself. Dropped.
…or-364 jsonapi: do not replace rsJsonApi silently, and assign it once
2 participants
it works now with Feedreader plugin but im not sure if it breaks something needs to be reviewed
@G10h4ck @thunder2 @csoler