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

Fix non-sending of error response on unsupported methods #71

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Seelengrab
Copy link
Contributor

@Seelengrab Seelengrab commented Oct 17, 2022

According to the JSON-RPC spec, the server MUST reply to all RPC calls (except notifications) with a response, in particular when an error occurs (see Section 5 of the JSON-RPC specification). JSONRPC.jl so far has not replied with error code -32601 Method not found when a request was made that is not supported. This PR fixes that, by replying with that error code when encountering a request that is not supported.

Since notifications do not have responses (and should thus be ignorable?), the server from this PR only replies to method requests by checking whether an id field exists or not.

On top of this, I've also consolidated the test in that file a bit, to make testing easier. A temporary directory is now created per test-run, to make testing on a developer machine easier by not leaving artifacts of the previous run behind. I've also prevented deadlocks in the tests, which could happen if the server_task got to the notify call before the main call got to the wait in rare instances (that always seemed to happen on my machine as soon as I expanded the tests).

Since this change should be purely under the hood, I don't think any additional end-user documentation for the VS Code extension or a changelog mention should be required.

It would have been nice to refer to the error code as a constant, but that requires #70 and will thus be done at a later date.

For every PR, please check the following:

@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Merging #71 (9682a93) into master (5adc04b) will increase coverage by 0.90%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   91.46%   92.36%   +0.90%     
==========================================
  Files           4        4              
  Lines         246      249       +3     
==========================================
+ Hits          225      230       +5     
+ Misses         21       19       -2     
Flag Coverage Δ
unittests 92.36% <100.00%> (+0.90%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/typed.jl 100.00% <100.00%> (ø)
src/core.jl 88.00% <0.00%> (+1.33%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Seelengrab
Copy link
Contributor Author

Bump :)

if is_request # even invalid requests MUST get a response - see Section 5 of the JSON RPC specification.
send_error_response(x, msg, -32601, "Unknown method '$method_name'.", nothing)
end
# TODO: ignoring notifications is legal, so there's no need to crash here on unknown things. However, removing this requires downstream support.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support in LS.jl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's in reference to something about the VSCode extension having built-in support for LS.jl crashing, for telemetry reasons (or so I recall from one of the issues on the LS.jl repo). I'm not familiar with how that works & interacts, but the assumption that the JSONRPC call throws an error is built into LS.jl, yes. Keeping the throwing behavior would need to be moved there, or changed to some other way entirely.

@Seelengrab
Copy link
Contributor Author

Seelengrab commented Mar 4, 2024

FYI, once #70 is merged, I'll rebase this onto it to no longer have the hard coded magic number here:

https://github.com/julia-vscode/JSONRPC.jl/pull/71/files#diff-9c8bdf42df2c37a8c2f4313ef26b85cddec870ac2aa63e601ef6fdf0c1f56941R83

I'll still have to look into downstream support so that the error call can be removed too. Ideally, the JSON-RPC server itself shouldn't ever crash, but rather refuse operation if it can't handle whatever is thrown at it.

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

Successfully merging this pull request may close these issues.

2 participants