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

Multiple Issues/Feedback #7

Closed
quinnj opened this issue May 5, 2015 · 5 comments
Closed

Multiple Issues/Feedback #7

quinnj opened this issue May 5, 2015 · 5 comments

Comments

@quinnj
Copy link
Member

quinnj commented May 5, 2015

Hey @tanmaykm, @viralshah

I'm a little slammed at the moment, but wanted to share some feedback and thoughts on taking this for a test drive for the last week:

  • For some reason, the logging functionality didn't work for me; I just ended up with empty .log files. I ended up putting println()s everywhere there were Logging actions and that worked really well to spin up two terminals and run the server in one and the invoker in another
  • It'd be nice to be able to Ctrl+C the Invoker (doing it to the server worked just fine). Just more convenient to Ctrl+C than have to kill the process
  • I had an issuer where I wasn't able to return non-JSON, the diff below fixed it for me
@@ -61,8 +64,12 @@ function get_resp(api::Nullable{APISpec}, status::Symbol, resp::Any=nothing)
     stcode = st[2]
     stresp = ((stcode != 0) && (resp === nothing)) ? "$(st[3]) : $(st[2])" : resp

-    if !isnull(api) && get(api).resp_json
-        return @compat Dict{String, Any}("code" => stcode, "data" => stresp)
+    if !isnull(api)
+        if get(api).resp_json
+            return JSON.json(stresp)
+        else
+            return stresp
+        end
     else
         return stresp
  • I had to change all the info calls to Logging.info because of the recent change to Base that kills both names when they clash
  • Doing some long-running connections, I often ran into the error below; not sure if there's a nicer way to handle this or how "bad" this is to encounter
ERROR (unhandled task failure): ArgumentError: stream is closed or unusable
 in check_open at stream.jl:301

A few questions:

  • How do request headers come through? Is there a way to access them from a function call? I ask because I'm interested in doing some basic authentication through header values (I guess I could do it through query parameters instead...)
  • How can you tell what HTTP method was used in invoking a call? I was only trying GET, but may want to shift to POST

Again, sorry for the mess here and not doing proper PRs, but wanted to share some feedback. Hopefully I'll be able to contribute more soon.

@tanmaykm
Copy link
Member

tanmaykm commented May 6, 2015

Thanks for the feedback @quinnj

Currently the Julia API methods are oblivious of the HTTP protocol. The idea was to have separate filters that map HTTP headers to method parameters, and keep the API methods independent of the protocol. Filters would also perform authentication, request logging and such tasks. Does this sound right?

Will check the stream is closed error. Was there any more of the exception stack that pointed to the source line?

@quinnj
Copy link
Member Author

quinnj commented May 6, 2015

The stream is closed error was in the APIInvoker and occurred when my client's stream closed prematurely before the API could generate the response.

One other point of feedback:

  • Maybe this was user error, but as is, I had to manually remove a few JSON.json/JSON.parse calls because they die on "invalid UTF8" data when I pass gzipped data as a string. If I pass it as a Vector{UInt8}, should that work? I realize I didn't try that, but on the other hand, it didn't seem to seriously break anything by just remove the JSON calls. Could they be removed for some efficiency?

@tanmaykm
Copy link
Member

tanmaykm commented May 7, 2015

@quinnj I had only meant to return HTML / JSON responses. But I think binary responses are a necessity for many applications. I think a little tweaking would enable that. Could you point out where you made the changes?

I shall push a few changes tomorrow, to fix some of the points discussed here.

tanmaykm added a commit that referenced this issue May 8, 2015
Logging API name clash with Base was making them inaccessible. ref #7
tanmaykm added a commit that referenced this issue May 9, 2015
binary content serving. sample code to demonstrate. ref #7
@tanmaykm
Copy link
Member

tanmaykm commented May 9, 2015

@quinnj please try binary responses with the latest changes. You should be able to pass gzipped data with this.
Also added some sample code to refer.

@aviks
Copy link
Member

aviks commented Jun 26, 2020

Much of this has been addressed, and others are probably state at this point. closing.

@aviks aviks closed this as completed Jun 26, 2020
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

No branches or pull requests

3 participants