maidsafe-archive / safe_launcher

Node.js App for SAFE Launcher
Other
95 stars 38 forks source link

Registering a long name & service (POST /dns) is not atomic #292

Closed bochaco closed 7 years ago

bochaco commented 7 years ago

I just tried to register a long name and a service with a POST /dns call (Launcher v0.9.2), providing an incorrect/non-existing serviceHomeDirPath, thus I get 404 error as I expected:

HTTP 404 Not Found { "errorCode": -1502, "description": "FfiError::PathNotFound" }

Then I corrected the serviceHomeDirPath to provide a valid and existing one, but I received a 400:

HTTP 400 Bad Request { "errorCode": -1001, "description": "DnsError::DnsNameAlreadyRegistered" }

The problem I see here is that the operation doesn't seem to be atomic, if the request fails it shouldn't register neither the service nor the long name. Or perhaps it should return a different HTTP status code?

ustulation commented 7 years ago

Ya most of the operations in the core lib are not atomic (it's difficult and/or time-consuming to have network atomic operations in our set-up - specially rollback of something if something else was unsuccessful - it just lead to more and more permutations like what if certain rollbacks were successful and others were not because of network issues etc.). We mostly aim for a self-healing Network and as long as we are not dead-locked it should be Ok i think. So in this case as DNS long-name is already registered we could use the other API - register-services (not the exact name) to register the service(s).

But ya agree on this one in the meanwhile:

Or perhaps it should return a different HTTP status code?

hitman401 commented 7 years ago

Previously safe_core used to expose a single function to registering a long name and service name. In the present implementation of safe_core this is removed. Couldn't remove from launcher APIs considering backward compatibility.

Validating the path provided before invoking the FFI function, should fix this issue.

Thanks @bochaco, will get this fixed in the next release.