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

Attempt fixing :sync #99

Closed
wants to merge 1 commit into from
Closed

Attempt fixing :sync #99

wants to merge 1 commit into from

Conversation

dickmao
Copy link
Collaborator

@dickmao dickmao commented Nov 30, 2018

See dickmao's comment in #92 regarding why :sync does not behave as
expected.

Previously, request--curl-sync would merely return as soon as the
curl process finished without regard to
#'request--curl-callback. This change attaches the semaphore to the
right place.

(Attempt) Fixes #92

See dickmao's comment in tkf#92 regarding why `:sync` does not behave as
expected.

Previously, `request--curl-sync` would merely return as soon as the
curl process finished *without regard to
`#'request--curl-callback`.  This change attaches the semaphore to the
right place.

(Attempt) Fixes tkf#92
@titaniumbones
Copy link
Collaborator

hmm, so the semaphore keeps checking in with the process about whether or not it's done? I'm a little confused still as to why async calls succeed when sync calls fail -- if there is still data to retrieve via accept-process-output shouldn't async calls also have trouble?

But then I don't understand async programmingvery well at all, esp not in elisp.

@alphapapa are you able to review this ?

@dickmao
Copy link
Collaborator Author

dickmao commented Nov 30, 2018

hmm, so the semaphore keeps checking in with the process about whether or not it's done?

No, the while-loop in request--curl-sync does that.

I'm a little confused still as to why async calls succeed when sync calls fail -- if there is still data to retrieve via accept-process-output shouldn't async calls also have trouble?

One, Async calls don't call request--curl-sync. Two, how do you define trouble? We don't make the same synchronous demands on async calls that we do :sync t calls.

Copy link
Collaborator

@titaniumbones titaniumbones left a comment

Choose a reason for hiding this comment

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

hmm, so the semaphore keeps checking in with the process about whether or not it's done?

No, the while-loop in request--curl-sync does that.

ah right. and semaphore is just another way to set finished to t, is that right?

I'm a little confused still as to why async calls succeed when sync calls fail -- if there is still data to retrieve via accept-process-output shouldn't async calls also have trouble?

One, Async calls don't call request--curl-sync. Two, how do you define trouble? We don't make the same synchronous demands on async calls that we do :sync t calls.

ok. since this is making poor wee head hurt... I will do what other people think is best. Are you able to pass all tests with yr branch?

@@ -1004,7 +1004,7 @@ temporary file paths."
files))

(cl-defun request--curl (url &rest settings
&key type data files headers timeout response
&key type data files headers timeout response semaphore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is "semaphore" a standard name for this kind of process watcher?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No? But "semaphore" is exactly what it is.

@dickmao
Copy link
Collaborator Author

dickmao commented Nov 30, 2018

I passed travis on the first attempt: https://travis-ci.org/tkf/emacs-request/builds/461595586

I appear not to be passing now. It a'int easy being green.

@alphapapa
Copy link

@alphapapa are you able to review this ?

I took a look and posted a few comments. I don't feel like I am familiar enough with Emacs's process-interacting code to pass judgement on it, but if @dickmao is, and if the tests pass, and if it seems to fix the bug, then great! :)

@titaniumbones
Copy link
Collaborator

@dickmao:

  • are you feeling good about the state of this PR? if so, would you please be willing to:
  • rebase on development and submit PR there? trying to stop pushing to master, esp when adding new features that have the potential to break older deployments.

@dickmao
Copy link
Collaborator Author

dickmao commented Dec 6, 2018

Done.

@dickmao dickmao closed this Dec 6, 2018
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.

3 participants