-
Notifications
You must be signed in to change notification settings - Fork 68
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: add line buffering logic to process filter #168
fix: add line buffering logic to process filter #168
Conversation
e3ae4d2
to
730d238
Compare
Matus Goljer ***@***.***> writes:
Recently I've submitted patch #167 to read messages from child processes.
The way it works is that it reads a base64 encoded "packet", decodes
it and if it is an async message it passes it further. However, when
the base64 string is too long, process filter can receive chunk of
data which is not a complete line and this will break the base64
decoding.
Yes, I thought there was such problem, but as long as the message are
short I thought it was enough.
I've added a decorator for the process filter which will buffer the
data until a full line is available and only then passes it to the
processor. This way you can send really long message packages such as
full error backtraces from worker processes to parent.
Generally the way to do this is to advance marker in the process buffer
and continue collecting output from it until process finishes.
I will merge your code and see later when I will have more time.
Thanks.
In case there is some copyright assignment issues, I took the
decorator code from my other package sallet so there should be no
issue.
Ok, I have no knowledge on these copyright issues, if someone see a
problem please comment. @johnwiegley @stefanmonnier ?
… ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
You can view, comment on, or merge this pull request online at:
#168
Commit Summary
• e3ae4d2 fix: add line buffering logic to process filter
File Changes
(1 file)
• M async.el (22)
Patch Links:
• https://github.com/jwiegley/emacs-async/pull/168.patch
• https://github.com/jwiegley/emacs-async/pull/168.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.*Message ID: ***@***.***>
--
Thierry
|
(with-current-buffer (process-buffer process) | ||
(insert string)) | ||
|
||
(let* ((line-data (split-string (concat data string) "\n"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we avoid all this consing and use some navigation or predicate functions instead? E.g.
- by comparing
(line-end-position)
with(line-end-position 2)
; or - checking
(char-after (line-end-position))
; or - using
count-lines
; or - using
eobp
; or...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, you need to also remember that:
- 0 new lines can come
- 1 new line can come
- many new lines can come.
The current code is simple (to read) and handles all these cases. For sure it could be made more efficient. For example, as @thierryvolpiatto said, we can keep one marker for the "last processed chunk" and then compare end of buffer against that, and when we find a newline inbetween, advance the marker line by line until there is no more newlines.
I think I will rewrite it like that, it sounds that it might be more efficient.
|
||
(let* ((line-data (split-string (concat data string) "\n"))) | ||
(while (cdr line-data) | ||
(funcall filter process (car line-data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means filter
will never see a newline, right? That doesn't seem right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might possibly be an issue if some other thing in async would rely on the newline. But the return value code does not and the "message" code also just reads a sexp which is one-line (base64) so it should be fine. I guess we can add a newline to the end of the data though.
Should I make a patch?
(set-process-filter proc #'async-read-from-client) | ||
(set-process-filter proc (async--process-filter-line-buffering-decorator | ||
#'async-read-from-client)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more commonly achieved using add-function
.
See for example M-x find-function RET shell-command RET C-M-e
.
(add-function :around (process-filter proc)
(lambda (filter proc output) ...))
Maybe you could even use :before-until
or :before-while
instead of :around
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly this seems rather complicated :O Does it really do the same thing? I'm using this factory to get a closure with the data
"buffer".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly this seems rather complicated :O
Does the intro in (info "(elisp) Advising Functions")
help demysticise it at all?
Does it really do the same thing?
Pretty much, but in a more flexible and introspectable way.
I'm using this factory to get a closure with the
data
"buffer".
You can make a similar closure with the :around
advice if you want to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check the info page, it sounds interesting. I'm only using advices through add-advice
which is probably a short hand for something else.
In the meantime, to fix #169 I rewrote the code using a marker as @thierryvolpiatto suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
advice-add
is implemented in terms of add-function
.
The former is for named function symbols, whereas the latter works on a variety of generalised variable places, such as process-filter
.
I'm not sure about the part "until process finishes". We specifically want to process the messages as soon as they could be processed to achieve a two-way communication. But you probably mean to "reset" some last-newline-marker every time and then gobble the code up-to there (or some such scheme, I'm sure I could figure it out :D) |
Recently I've submitted patch #167 to read messages from child processes.
The way it works is that it reads a base64 encoded "packet", decodes it and if it is an async message it passes it further. However, when the base64 string is too long, process filter can receive chunk of data which is not a complete line and this will break the base64 decoding.
I've added a decorator for the process filter which will buffer the data until a full line is available and only then passes it to the processor. This way you can send really long message packages such as full error backtraces from worker processes to parent.
In case there is some copyright assignment issues, I took the decorator code from my other package sallet so there should be no issue.