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

Return errors from terminatesession and traverseiterator in some cases #745

Open
roman-khimov opened this issue Jul 20, 2022 · 2 comments
Open

Comments

@roman-khimov
Copy link
Contributor

Summary or problem description
We have the following cases for terminatesession and traverseiterator RPCs that can be improved IMO:

  • SessionEnabled: false
    terminatesession will always return false in this case while traverseiterator will always return an empty array. While not a big problem for terminatesession this behavior is far more annoying for traverseiterator, especially on public servers that are not expected to have sessions enabled for security reasons. These servers will return session/iterator IDs from invoke*, but clients wouldn't be able to distinguish between iterator that has no elements and server that doesn't have sessions enabled.
  • invalid session IDs
    The behavior now is similar to SessionEnabled: false. And it's important for the client to separate session errors (client can use wrong ID or it may just expire) from empty iterator again.
  • invalid iterator IDs in traverseiterator
    The behavior now is similar to invalid session ID and it again is important for the client to know that the ID is invalid if that's the case.

Do you have any solution you want to propose?
Introduce and return new errors for the cases described above (see #728 also for specific code suggestions, although for this issue it's more important to have these errors handled as errors in the first place).

Where in the software does this update applies to?

  • RPC (HTTP)
@erikzhang
Copy link
Member

These servers will return session/iterator IDs from invoke*

if (session.Iterators.Count == 0 || !settings.SessionEnabled)
{
session.Dispose();
}
else
{
Guid id = Guid.NewGuid();
json["session"] = id.ToString();
lock (sessions)
sessions.Add(id, session);
}

It won't return session id if SessionEnabled is false.

And it's important for the client to separate session errors (client can use wrong ID or it may just expire) from empty iterator again.

session = sessions[sid];

IIterator iterator = session.Iterators[iid];

If sid or iid is wrong, it will throw a KeyNotFoundException.

@roman-khimov
Copy link
Contributor Author

It won't return session id if SessionEnabled is false.

True, thanks. It returns iterator ID, but not session ID. But iterator ID is still somewhat misleading IMO, so separating SessionEnabled: false case is still worth doing. In case user tries to invoke any of these methods a special error will at least clearly communicate that this won't work with any IDs.

If sid or iid is wrong, it will throw a KeyNotFoundException.

This then leads to interesting replies:

$ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "traverseiterator", "params": [] }'...
{
   "id" : 1,
   "error" : {
      "message" : "Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index')",
      "code" : -2146233086
   },
   "jsonrpc" : "2.0"
}
$ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "traverseiterator", "params": ["BCBA19D9-F868-4C6D-8061-A2B91E06E3EC", "BCBA19D9-F868-4C6D-8061-A2B91E06E3EC", 1] }'...
{
   "jsonrpc" : "2.0",
   "id" : 1,
   "error" : {
      "code" : -2146232969,
      "message" : "The given key 'bcba19d9-f868-4c6d-8061-a2b91e06e3ec' was not present in the dictionary."
   }
}

Error codes are subject of #728 though, so the only question left here is SessionEnabled: false.

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

No branches or pull requests

2 participants