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

Replaced Old Apache HTTP Libraries with OkHttpClient #69

Merged
merged 6 commits into from Jul 11, 2016

Conversation

Projects
None yet
4 participants
@kunall17
Copy link
Collaborator

kunall17 commented Jun 19, 2016

Espresso Test's are already included in #45 and #48!

@smarx

This comment has been minimized.

Copy link

smarx commented Jun 19, 2016

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

HttpDeleteWithReq(String url) {
super(url);
public Response execute(String method, String path) throws IOException {
if ("".equals(method)) throw new IOException(app.getString(R.string.method_null));

This comment has been minimized.

@niftynei

niftynei Jun 21, 2016

Contributor

does this also check for nulls?

Log.i("HTTP.response", "<empty>");
}
return responseString;
return encodedUrl.substring(0, encodedUrl.length() - 1);

This comment has been minimized.

@niftynei

niftynei Jun 21, 2016

Contributor

there's probably a safer way to remove the last character only if it's an &.

protected void handleHTTPError(HttpResponseException e) {
if (e.getStatusCode() < 500 && e.getStatusCode() >= 400) {
protected void handleHTTPError(Exception e) {
// if (e.getStatusCode() < 500 && e.getStatusCode() >= 400) {

This comment has been minimized.

@niftynei

niftynei Jun 21, 2016

Contributor

If this code is unused, it should be deleted. :)

return request.execute(api_path[0], api_path[1]);
} catch (HttpResponseException e) {
handleHTTPError(e);
Response response = request.execute(api_path[0], api_path[1]);

This comment has been minimized.

@niftynei

niftynei Jun 21, 2016

Contributor

is it possible that we don't pass enough arguments to this array?

@@ -96,7 +85,7 @@ public void run() {
}
});
// supermethod invokes cancel for us
super.handleHTTPError(e);
// super.handleHTTPError(e);

This comment has been minimized.

@niftynei

niftynei Jun 21, 2016

Contributor

can remove this if it's unused.

} catch (HttpResponseException e) {
if (e.getStatusCode() == 400) {
String msg = e.getMessage();
if (httpResponse.code() == 400) {

This comment has been minimized.

@niftynei

niftynei Jun 21, 2016

Contributor

may need to check the response code for success earlier, since it no longer throws an error.

return null;
}
}).execute();
}
}

// Java doesn't support a request body on DELETE requests, but RFC2616 does
// not prohibit it.
class HttpDeleteWithReq extends HttpPost {

This comment has been minimized.

@niftynei

niftynei Jun 21, 2016

Contributor

Here the Delete code extends from POST, but in your updated version, it appears to not support this behavior?

} catch (HttpResponseException e) {
handleHTTPError(e);
Response response = request.execute(api_path[0], api_path[1]);
return response.body().string();

This comment has been minimized.

@niftynei

niftynei Jun 21, 2016

Contributor

this might be a good place to check for http error status codes being returned, and calling handleHTTPError.

@niftynei

This comment has been minimized.

Copy link
Contributor

niftynei commented Jun 21, 2016

I'm getting this error when running it:

06-21 17:21:08.528 E/Error   (18516): oops
06-21 17:21:08.528 E/Error   (18516): org.json.JSONException: Value <html><title>500 of type java.lang.String cannot be converted to JSONObject
06-21 17:21:08.528 E/Error   (18516):   at org.json.JSON.typeMismatch(JSON.java:111)
06-21 17:21:08.528 E/Error   (18516):   at org.json.JSONObject.<init>(JSONObject.java:160)
06-21 17:21:08.528 E/Error   (18516):   at org.json.JSONObject.<init>(JSONObject.java:173)
06-21 17:21:08.528 E/Error   (18516):   at com.zulip.android.networking.AsyncGetEvents.run(AsyncGetEvents.java:114)
@niftynei

This comment has been minimized.

Copy link
Contributor

niftynei commented Jun 21, 2016

Overall this looks good, it needs to handle HTTP errors better (which is what's causing the bug above).

From a UI perspective, it shows me a "Message Sent" toast even though the message isn't actually sent. This might be better addressed in a separate issue tho.

@niftynei

This comment has been minimized.

Copy link
Contributor

niftynei commented Jun 21, 2016

Note that this is related to issue #29

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from 6079d9f to 6c6a1fd Jun 22, 2016

@@ -155,6 +161,7 @@ public void run() {
} catch (JSONException e) {
backoff(e);
}

This comment has been minimized.

@kunall17

kunall17 Jun 22, 2016

Author Collaborator

Without Thread.sleep I receive a traceback on my server

AssertionError
2016-06-22 17:00:08,438 INFO     127.0.0.1       GET     429  11ms /api/v1/events (iago@zulip.com via okhttp)
2016-06-22 17:00:08,439 INFO     status=429, data={"msg":"API usage exceeded rate limit, try again in 36.4631140232 secs","result":"error"}
, uid=iago@zulip.com
2016-06-22 17:00:08,441 ERROR    Uncaught exception GET /api/v1/events?queue_id=1466627219%3A4&last_event_id=19& (127.0.0.1)
HTTPRequest(protocol='http', host='localhost:9993', method='GET', uri='/api/v1/events?queue_id=1466627219%3A4&last_event_id=19&', version='HTTP/1.0', remote_ip='127.0.0.1', body='', headers={'Host': 'localhost:9993', 'Client': 'Android', 'Accept-Encoding': 'gzip', 'X-Forwarded-Host': '192.168.100.6:9991', 'Connection': 'close', 'Authorization': 'Basic aWFnb0B6dWxpcC5jb206bVJHRjZ1Nmt4V0RBVHo2NU1VbWJXa3AxUzZrSk9mRG0=', 'User-Agent': 'okhttp/3.2.0'})
Traceback (most recent call last):
  File "/srv/zulip-venv-cache/615b1182e7545b42f90c5ecfcad3168c4ccb6246/zulip-venv/local/lib/python2.7/site-packages/tornado/web.py", line 1042, in _execute
    getattr(self, self.request.method.lower())(*args, **kwargs)
  File "/srv/zulip/zerver/management/commands/runtornado.py", line 184, in get
    self.set_status(response.status_code)
  File "/srv/zulip-venv-cache/615b1182e7545b42f90c5ecfcad3168c4ccb6246/zulip-venv/local/lib/python2.7/site-packages/tornado/web.py", line 245, in set_status
    assert status_code in httplib.responses

I guess this is because OkHttpClient is running faster than the existing Apache HTTP Clients!
We might have to increase/decrease the sleep time!

This comment has been minimized.

@kunall17

kunall17 Aug 2, 2016

Author Collaborator

@timabbott After changing from Apache HTTP to okHttpClient asyncGetEvents for the queue works a lot faster and hence it floods the server, so currently I gave a 1 sec pause after every heartbeat receives, will this be the right interval time or increasing this will also do fine?
The current app seems fine though in receiving new messages!

This comment has been minimized.

@timabbott

timabbott Aug 2, 2016

Member

What we do in the web UI is this sort of exponential backoff if the client receives an error from the server (and no backoff other than the fact that the client uses longpolling (aka doesn't pass dont_block=True and thus is rate-limited by new events showing up at the server if it receives a success response). Here's the code for the exponential backoff size: https://github.com/zulip/zulip/blob/master/static/js/server_events.js#L353

Note that in this case we also show a user-facing error message overlay over the website that explains the client can't connect to the server and with a "try now" button to avoid the user having to wait up to 90s in the event that they've been off the network for a while and just got back on.

You can see how this looks by logging into a Zulip development server and then running ctrl-C on run-dev.py and watching.

(Since this is a complicated explanation, let me know if you have any questions!)

@kunall17

This comment has been minimized.

Copy link
Collaborator Author

kunall17 commented Jun 22, 2016

@niftynei Updated the PR, Rebased to master, fixed bugs in sending a private message, added Logs to get the URL and JSON, Added error handling in pushTask!

Response httpResponse = request.execute();

String json = httpResponse.body().string();
if (!httpResponse.isSuccessful()) {

This comment has been minimized.

@niftynei

niftynei Jun 23, 2016

Contributor

It might be worth also checking the status code?

This comment has been minimized.

@kunall17

kunall17 Jun 24, 2016

Author Collaborator

Before fetching the String by String json = httpResponse.body().string(); ??



<string name="method_null">Method should not be null</string>
<string name="method_error">Method Not Supported</string>

This comment has been minimized.

@niftynei

niftynei Jun 23, 2016

Contributor

Can you make this lower case? like Message not supported ?

case "POST":
FormBody.Builder formBody = new FormBody.Builder();
for (Map.Entry<String, String> map : properties.entrySet()) {
formBody.addEncoded(map.getKey(), map.getValue());

This comment has been minimized.

@niftynei

niftynei Jun 23, 2016

Contributor

I'm running into a problem when logging in with an email. This API addEncoded is transforming the @ into the encoded version %40, making it such that I can't log in. :(

This comment has been minimized.

@kunall17

kunall17 Jun 24, 2016

Author Collaborator

Could not reproduce this problem, I am able to successfully login with the existing code (tried with zulip.tabbott.net as well)
But yeah these strings should not be encoded so I updated the code but it's weird why was it working with my build with the encodedPaths and it works in both the ways, without and with encoding!

@niftynei

This comment has been minimized.

Copy link
Contributor

niftynei commented Jun 23, 2016

Cool. It's working a lot better now 👍 .

I encountered two problems tho:

  1. Logging in with an incorrect username & password doesn't show an error message. 😞
  2. commented in-line as well, but the form encoding is making it such that emails aren't being passed up to the server correctly. maybe we can turn URI encoding off?

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from 6c6a1fd to e50487c Jun 24, 2016

@kunall17

This comment has been minimized.

Copy link
Collaborator Author

kunall17 commented Jun 24, 2016

  1. Logging in with an incorrect username & password doesn't show an error message. 😞

I had to discuss on this one, In this PR if the JSON is recieved and is not successful (status code not lies in 200) then it goes to onTaskFailed in the callback, I think this is more better as the task is failed (task failed for example incorrect login credentials) and previously it went to the onTaskCompleted even if the JSON had a status code on non 200's, which way you think is more liable to be?

This line creates the difference https://github.com/kunall17/zulip-android/blob/e50487c6a94ba8586f9d9181047af2e36e63a803/app/src/main/java/com/zulip/android/networking/ZulipAsyncPushTask.java#L109

@niftynei

This comment has been minimized.

Copy link
Contributor

niftynei commented Jul 7, 2016

@kunall17 Failing the task sounds reasonable to me, as long as you can preserve the error messages appropriately. I checked this out again though and it still doesn't show an error message with incorrect credentials. Is this supposed to be fixed?

Managed to log in with out a problem this time, thanks for fixing that encoding error! 👍

If you can fix up the error messages & rebase this to master (the DevBackends code is still using the old style HTTP stuff), I'll get this merged. Thanks!

@niftynei niftynei closed this Jul 7, 2016

@niftynei niftynei reopened this Jul 7, 2016

@kunall17

This comment has been minimized.

Copy link
Collaborator Author

kunall17 commented Jul 8, 2016

@niftynei yeah I left it purposely here, I will complete this and rebase it!

@kunall17 kunall17 force-pushed the kunall17:patch-5 branch from e50487c to 5aa39fd Jul 9, 2016

@kunall17

This comment has been minimized.

Copy link
Collaborator Author

kunall17 commented Jul 9, 2016

@niftynei updated the code!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.