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 #597] Don't process incomplete messages unless sure #616

Merged
merged 1 commit into from
Jun 11, 2014

Conversation

vspinu
Copy link
Contributor

@vspinu vspinu commented Jun 11, 2014

The bencode decoder started driving me nuts. So, I decided to have a look and it turned to be a simple fix. It is still not a full proof solution but, it covers 99.99% of cases without wasting decoding cycles on incomplete messages.

It probably also fixes the related #583 and #586.

@bbatsov
Copy link
Member

bbatsov commented Jun 11, 2014

While we do need a solution for this problem - this is not the way. As I said - delay based solutions are notoriously buggy. We have quite a few similar "workarounds" in the code and want to get rid of all of them. I think that a more sensible approach would be checking the length of the response - nREPL streams the responses in 4096 byte chunks, so I guess we should wait until we get a response that's less than 4096 bytes.

@cemerick Is there some recommended way to check whether a bencoded message has been fully received?

@@ -14,7 +14,8 @@ for presenting fontified documentation, including Javadoc.
* `cider-select` can now switch to the `*cider-error*` buffer (bound to `x`).

### Changes

* [#597](https://github.com/clojure-emacs/cider/issues/597) Don't process reple
Copy link
Member

Choose a reason for hiding this comment

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

reple -> reply

Perhaps response would be even better.

@vspinu
Copy link
Contributor Author

vspinu commented Jun 11, 2014

The solution is not in the delay but in the check for the dict end. The delay is to catch those rare ocassions when "e" in the middle of the message turned to be at the end of the received string.

The point of the delay is to quickly check if the process is piping new data, if it does, we wait and don't start decoding because it is meaningless. There is nothing fragile about it and I am pretty sure there is no better way to check for an incomplete message without reparsing the whole message.

The delay is minuscul and it works even with .001 if you want. The process returns earlier if the new string has arrived earlier, so there is no waiting during the reception of the message, only on completion.

As I said earlier this will handle most of the cases. You can think of further enhancing it but, I bet it won't be necessary. Simple fix and it does the job with no side effects. I have implemented something similar for ESS years ago and have never had any problems.

(dolist (r responses)
(nrepl-dispatch r))))))))

(defvar nrepl-wait-before-processing 0.01
Copy link
Member

Choose a reason for hiding this comment

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

wait-before-processing -> output-timeout perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well may be decode-timeout because it is the time before starting the deccodding?

@bbatsov
Copy link
Member

bbatsov commented Jun 11, 2014

@Vitoshka OK, I'm sold. I re-read the code more carefully and I think it will get the job done indeed. Address my minor remarks and I'll have it merged.

Btw, this doesn't seem related to #583 because there the problem is caused by unencoded text ending up in the connection buffer (I still haven't figured out how this happens in the first place).

@vspinu
Copy link
Contributor Author

vspinu commented Jun 11, 2014

Ok. done! Really glad you are accepting it :)

bbatsov added a commit that referenced this pull request Jun 11, 2014
[Fix #597] Don't process incomplete messages unless sure
@bbatsov bbatsov merged commit 112550d into clojure-emacs:master Jun 11, 2014
@bbatsov
Copy link
Member

bbatsov commented Jun 11, 2014

Thanks!

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