channel: Remove idempotent retry loop.

This was added by commit 7f174213ed, and
appears to have been designed for responses that are *successful* but
falsy. Logically, these should not implicitly represent a failure to
be retried if it were.

Note from tabbott: The background is that this idempotent retry loop
was a hacky workaround for a bug we never understood but saw daily in
production. Especially during server restarts / client reloads,
something would result in 200 responses with no data being seen by the
frontend, despite the Django server not having received/processed the
request. Fortunately, this strange failure mode appears to have
stopped happening in late 2019, so we can delete this hack.

Signed-off-by: Anders Kaseorg <anders@zulip.com>
This commit is contained in:
Anders Kaseorg
2022-08-18 19:45:06 -07:00
committed by Tim Abbott
parent 4391bc324b
commit fde9b1d366
25 changed files with 6 additions and 80 deletions

View File

@@ -9,11 +9,6 @@ const {run_test} = require("../zjsunit/test");
const blueslip = require("../zjsunit/zblueslip"); const blueslip = require("../zjsunit/zblueslip");
const {page_params} = require("../zjsunit/zpage_params"); const {page_params} = require("../zjsunit/zpage_params");
set_global("setTimeout", (f, delay) => {
assert.equal(delay, 0);
f();
});
const xhr_401 = { const xhr_401 = {
status: 401, status: 401,
responseText: '{"msg": "Use cannot access XYZ"}', responseText: '{"msg": "Use cannot access XYZ"}',
@@ -336,30 +331,6 @@ test("unexpected_403_response", () => {
}); });
}); });
test("retry", () => {
test_with_mock_ajax({
run_code() {
channel.post({
idempotent: true,
data: 42,
});
},
check_ajax_options(options) {
blueslip.expect("log", "Retrying idempotent[object Object]");
test_with_mock_ajax({
run_code() {
options.simulate_success();
},
check_ajax_options(options) {
assert.equal(options.data, 42);
},
});
},
});
});
test("too_many_pending", () => { test("too_many_pending", () => {
channel.clear_for_tests(); channel.clear_for_tests();
$.ajax = () => { $.ajax = () => {

View File

@@ -808,7 +808,6 @@ test_ui("on_events", ({override, override_rewire}) => {
override(channel, "post", (payload) => { override(channel, "post", (payload) => {
assert.equal(payload.url, "/json/messages/render"); assert.equal(payload.url, "/json/messages/render");
assert.ok(payload.idempotent);
assert.ok(payload.data); assert.ok(payload.data);
assert.deepEqual(payload.data.content, current_message); assert.deepEqual(payload.data.content, current_message);

View File

@@ -141,7 +141,6 @@ run_test("unread_ops", ({override}) => {
// url and parameters are specified: // url and parameters are specified:
assert.deepEqual(channel_post_opts, { assert.deepEqual(channel_post_opts, {
url: "/json/messages/flags", url: "/json/messages/flags",
idempotent: true,
data: {messages: "[50]", op: "add", flag: "read"}, data: {messages: "[50]", op: "add", flag: "read"},
success: channel_post_opts.success, success: channel_post_opts.success,
}); });

View File

@@ -169,7 +169,6 @@ run_test("read", ({override}) => {
send_read(msgs_to_flag_read); send_read(msgs_to_flag_read);
assert.deepEqual(channel_post_opts, { assert.deepEqual(channel_post_opts, {
url: "/json/messages/flags", url: "/json/messages/flags",
idempotent: true,
data: { data: {
messages: "[1,2,3,4,5]", messages: "[1,2,3,4,5]",
op: "add", op: "add",
@@ -185,7 +184,6 @@ run_test("read", ({override}) => {
channel_post_opts.success(success_response_data); channel_post_opts.success(success_response_data);
assert.deepEqual(channel_post_opts, { assert.deepEqual(channel_post_opts, {
url: "/json/messages/flags", url: "/json/messages/flags",
idempotent: true,
data: { data: {
messages: "[6,7]", messages: "[6,7]",
op: "add", op: "add",
@@ -213,7 +211,6 @@ run_test("read", ({override}) => {
send_read(msgs_to_flag_read); send_read(msgs_to_flag_read);
assert.deepEqual(channel_post_opts, { assert.deepEqual(channel_post_opts, {
url: "/json/messages/flags", url: "/json/messages/flags",
idempotent: true,
data: { data: {
messages: "[3,4,5,6,7]", messages: "[3,4,5,6,7]",
op: "add", op: "add",
@@ -252,7 +249,6 @@ run_test("read", ({override}) => {
// Former locally echoed messages flagging retried // Former locally echoed messages flagging retried
assert.deepEqual(channel_post_opts, { assert.deepEqual(channel_post_opts, {
url: "/json/messages/flags", url: "/json/messages/flags",
idempotent: true,
data: { data: {
messages: "[1,2]", messages: "[1,2]",
op: "add", op: "add",
@@ -304,7 +300,6 @@ run_test("collapse_and_uncollapse", ({override}) => {
assert.deepEqual(channel_post_opts, { assert.deepEqual(channel_post_opts, {
url: "/json/messages/flags", url: "/json/messages/flags",
idempotent: true,
data: { data: {
messages: "[5]", messages: "[5]",
op: "add", op: "add",
@@ -316,7 +311,6 @@ run_test("collapse_and_uncollapse", ({override}) => {
assert.deepEqual(channel_post_opts, { assert.deepEqual(channel_post_opts, {
url: "/json/messages/flags", url: "/json/messages/flags",
idempotent: true,
data: { data: {
messages: "[5]", messages: "[5]",
op: "remove", op: "remove",

View File

@@ -197,7 +197,6 @@ export function send_presence_to_server(want_redraw) {
new_user_input, new_user_input,
slim_presence: true, slim_presence: true,
}, },
idempotent: true,
success(data) { success(data) {
// Update Zephyr mirror activity warning // Update Zephyr mirror activity warning
if (data.zephyr_mirror_active === false) { if (data.zephyr_mirror_active === false) {

View File

@@ -53,7 +53,6 @@ function delete_attachments(attachment) {
const $status = $("#delete-upload-status"); const $status = $("#delete-upload-status");
channel.del({ channel.del({
url: "/json/attachments/" + attachment, url: "/json/attachments/" + attachment,
idempotent: true,
error(xhr) { error(xhr) {
ui_report.error($t_html({defaultMessage: "Failed"}), xhr, $status); ui_report.error($t_html({defaultMessage: "Failed"}), xhr, $status);
}, },
@@ -152,7 +151,6 @@ export function set_up_attachments() {
channel.get({ channel.get({
url: "/json/attachments", url: "/json/attachments",
idempotent: true,
success(data) { success(data) {
loading.destroy_indicator($("#attachments_loading_indicator")); loading.destroy_indicator($("#attachments_loading_indicator"));
format_attachment_data(data.attachments); format_attachment_data(data.attachments);

View File

@@ -39,7 +39,7 @@ function remove_pending_request(jqXHR) {
} }
} }
function call(args, idempotent) { function call(args) {
if (reload_state.is_in_progress() && !args.ignore_reload) { if (reload_state.is_in_progress() && !args.ignore_reload) {
// If we're in the process of reloading, most HTTP requests // If we're in the process of reloading, most HTTP requests
// are useless, with exceptions like cleaning up our event // are useless, with exceptions like cleaning up our event
@@ -128,15 +128,6 @@ function call(args, idempotent) {
return; return;
} }
if (!data && idempotent) {
// If idempotent, retry
blueslip.log("Retrying idempotent" + args);
setTimeout(() => {
const jqXHR = $.ajax(args);
add_pending_request(jqXHR);
}, 0);
return;
}
orig_success(data, textStatus, jqXHR); orig_success(data, textStatus, jqXHR);
}; };
@@ -154,23 +145,23 @@ function call(args, idempotent) {
export function get(options) { export function get(options) {
const args = {type: "GET", dataType: "json", ...options}; const args = {type: "GET", dataType: "json", ...options};
return call(args, options.idempotent); return call(args);
} }
export function post(options) { export function post(options) {
const args = {type: "POST", dataType: "json", ...options}; const args = {type: "POST", dataType: "json", ...options};
return call(args, options.idempotent); return call(args);
} }
export function put(options) { export function put(options) {
const args = {type: "PUT", dataType: "json", ...options}; const args = {type: "PUT", dataType: "json", ...options};
return call(args, options.idempotent); return call(args);
} }
// Not called exports.delete because delete is a reserved word in JS // Not called exports.delete because delete is a reserved word in JS
export function del(options) { export function del(options) {
const args = {type: "DELETE", dataType: "json", ...options}; const args = {type: "DELETE", dataType: "json", ...options};
return call(args, options.idempotent); return call(args);
} }
export function patch(options) { export function patch(options) {
@@ -183,7 +174,7 @@ export function patch(options) {
} else { } else {
options.data = {...options.data, method: "PATCH"}; options.data = {...options.data, method: "PATCH"};
} }
return post(options, options.idempotent); return post(options);
} }
export function xhr_error_message(message, xhr) { export function xhr_error_message(message, xhr) {

View File

@@ -381,7 +381,6 @@ export function render_and_show_preview($preview_spinner, $preview_content_box,
} }
channel.post({ channel.post({
url: "/json/messages/render", url: "/json/messages/render",
idempotent: true,
data: {content}, data: {content},
success(response_data) { success(response_data) {
if (markdown.contains_backend_only_syntax(content)) { if (markdown.contains_backend_only_syntax(content)) {

View File

@@ -566,7 +566,6 @@ export function quote_and_reply(opts) {
channel.get({ channel.get({
url: "/json/messages/" + message_id, url: "/json/messages/" + message_id,
idempotent: true,
success(data) { success(data) {
message.raw_content = data.raw_content; message.raw_content = data.raw_content;
replace_content(message); replace_content(message);

View File

@@ -705,7 +705,6 @@ export function start($row, edit_box_open_callback) {
const msg_list = message_lists.current; const msg_list = message_lists.current;
channel.get({ channel.get({
url: "/json/messages/" + message.id, url: "/json/messages/" + message.id,
idempotent: true,
success(data) { success(data) {
if (message_lists.current === msg_list) { if (message_lists.current === msg_list) {
message.raw_content = data.raw_content; message.raw_content = data.raw_content;
@@ -1232,7 +1231,6 @@ export function with_first_message_id(stream_id, topic_name, success_cb, error_c
channel.get({ channel.get({
url: "/json/messages", url: "/json/messages",
data, data,
idempotent: true,
success(data) { success(data) {
const message_id = data.messages[0].id; const message_id = data.messages[0].id;
success_cb(message_id); success_cb(message_id);

View File

@@ -261,7 +261,6 @@ export function load_messages(opts) {
channel.get({ channel.get({
url: "/json/messages", url: "/json/messages",
data, data,
idempotent: true,
success(data) { success(data) {
get_messages_success(data, opts); get_messages_success(data, opts);
}, },

View File

@@ -9,7 +9,6 @@ import * as unread_ops from "./unread_ops";
function send_flag_update_for_messages(msg_ids, flag, op) { function send_flag_update_for_messages(msg_ids, flag, op) {
channel.post({ channel.post({
url: "/json/messages/flags", url: "/json/messages/flags",
idempotent: true,
data: { data: {
messages: JSON.stringify(msg_ids), messages: JSON.stringify(msg_ids),
flag, flag,
@@ -40,7 +39,6 @@ export const send_read = (function () {
channel.post({ channel.post({
url: "/json/messages/flags", url: "/json/messages/flags",
idempotent: true,
data: {messages: JSON.stringify(real_msg_ids_batch), op: "add", flag: "read"}, data: {messages: JSON.stringify(real_msg_ids_batch), op: "add", flag: "read"},
success: on_success, success: on_success,
}); });
@@ -140,7 +138,6 @@ export function unstar_all_messages_in_topic(stream_id, topic) {
channel.get({ channel.get({
url: "/json/messages", url: "/json/messages",
data, data,
idempotent: true,
success(data) { success(data) {
const messages = data.messages; const messages = data.messages;
const starred_message_ids = messages.map((message) => message.id); const starred_message_ids = messages.map((message) => message.id);

View File

@@ -54,7 +54,6 @@ export function mute_topic(stream_id, topic, from_hotkey) {
channel.patch({ channel.patch({
url: "/json/users/me/subscriptions/muted_topics", url: "/json/users/me/subscriptions/muted_topics",
idempotent: true,
data, data,
success() { success() {
if (!from_hotkey) { if (!from_hotkey) {
@@ -96,7 +95,6 @@ export function unmute_topic(stream_id, topic) {
channel.patch({ channel.patch({
url: "/json/users/me/subscriptions/muted_topics", url: "/json/users/me/subscriptions/muted_topics",
idempotent: true,
data, data,
success() { success() {
feedback_widget.dismiss(); feedback_widget.dismiss();

View File

@@ -16,7 +16,6 @@ import * as settings_muted_users from "./settings_muted_users";
export function mute_user(user_id) { export function mute_user(user_id) {
channel.post({ channel.post({
url: "/json/users/me/muted_users/" + user_id, url: "/json/users/me/muted_users/" + user_id,
idempotent: true,
}); });
} }
@@ -40,7 +39,6 @@ export function confirm_mute_user(user_id) {
export function unmute_user(user_id) { export function unmute_user(user_id) {
channel.del({ channel.del({
url: "/json/users/me/muted_users/" + user_id, url: "/json/users/me/muted_users/" + user_id,
idempotent: true,
}); });
} }

View File

@@ -353,7 +353,6 @@ export function activate(raw_operators, opts) {
// for it. // for it.
channel.get({ channel.get({
url: `/json/messages/${id_info.target_id}`, url: `/json/messages/${id_info.target_id}`,
idempotent: true,
success(data) { success(data) {
// After the message is fetched, we make the // After the message is fetched, we make the
// message locally available and then call // message locally available and then call

View File

@@ -176,7 +176,6 @@ export function initialize() {
return channel.patch({ return channel.patch({
url: "/json/settings", url: "/json/settings",
idempotent: true,
data: {enter_sends: selected_behaviour}, data: {enter_sends: selected_behaviour},
}); });
}); });

View File

@@ -20,7 +20,6 @@ export function show_user_list(message_id) {
loading.make_indicator($("#read_receipts_modal .loading_indicator")); loading.make_indicator($("#read_receipts_modal .loading_indicator"));
channel.get({ channel.get({
url: `/json/messages/${message_id}/read_receipts`, url: `/json/messages/${message_id}/read_receipts`,
idempotent: true,
success(data) { success(data) {
const users = data.user_ids.map((id) => { const users = data.user_ids.map((id) => {
const user = people.get_by_user_id(id); const user = people.get_by_user_id(id);

View File

@@ -125,7 +125,6 @@ export function do_set_reminder_for_message(message_id, timestamp) {
const msg_list = message_lists.current; const msg_list = message_lists.current;
channel.get({ channel.get({
url: "/json/messages/" + message.id, url: "/json/messages/" + message.id,
idempotent: true,
success(data) { success(data) {
if (message_lists.current === msg_list) { if (message_lists.current === msg_list) {
message.raw_content = data.raw_content; message.raw_content = data.raw_content;

View File

@@ -205,7 +205,6 @@ function get_events({dont_block = false} = {}) {
get_events_xhr = channel.get({ get_events_xhr = channel.get({
url: "/json/events", url: "/json/events",
data: get_events_params, data: get_events_params,
idempotent: true,
timeout: page_params.event_queue_longpoll_timeout_seconds * 1000, timeout: page_params.event_queue_longpoll_timeout_seconds * 1000,
success(data) { success(data) {
watchdog.set_suspect_offline(false); watchdog.set_suspect_offline(false);

View File

@@ -556,7 +556,6 @@ export function set_up() {
const bot_id = Number.parseInt($(e.currentTarget).attr("data-user-id"), 10); const bot_id = Number.parseInt($(e.currentTarget).attr("data-user-id"), 10);
channel.post({ channel.post({
url: "/json/bots/" + encodeURIComponent(bot_id) + "/api_key/regenerate", url: "/json/bots/" + encodeURIComponent(bot_id) + "/api_key/regenerate",
idempotent: true,
success(data) { success(data) {
const $row = $(e.currentTarget).closest("li"); const $row = $(e.currentTarget).closest("li");
$row.find(".api_key").find(".value").text(data.api_key); $row.find(".api_key").find(".value").text(data.api_key);

View File

@@ -168,7 +168,6 @@ export function set_up(initialize_event_handlers = true) {
// Populate invites table // Populate invites table
channel.get({ channel.get({
url: "/json/invites", url: "/json/invites",
idempotent: true,
timeout: 10 * 1000, timeout: 10 * 1000,
success(data) { success(data) {
on_load_success(data, initialize_event_handlers); on_load_success(data, initialize_event_handlers);

View File

@@ -431,7 +431,6 @@ export function confirm_deactivation(user_id, handle_confirm, loading_spinner) {
// request fails. // request fails.
channel.get({ channel.get({
url: "/json/invites", url: "/json/invites",
idempotent: true,
timeout: 10 * 1000, timeout: 10 * 1000,
success(data) { success(data) {
let number_of_invites_by_user = 0; let number_of_invites_by_user = 0;

View File

@@ -1041,7 +1041,6 @@ function get_chart_data(data, callback) {
$.get({ $.get({
url: "/json/analytics/chart_data" + page_params.data_url_suffix, url: "/json/analytics/chart_data" + page_params.data_url_suffix,
data, data,
idempotent: true,
success(data) { success(data) {
callback(data); callback(data);
update_last_full_update(data.end_times); update_last_full_update(data.end_times);

View File

@@ -18,7 +18,6 @@ export function mark_all_as_read() {
channel.post({ channel.post({
url: "/json/mark_all_as_read", url: "/json/mark_all_as_read",
idempotent: true,
success: () => { success: () => {
// After marking all messages as read, we reload the browser. // After marking all messages as read, we reload the browser.
// This is useful to avoid leaving ourselves deep in the past. // This is useful to avoid leaving ourselves deep in the past.
@@ -183,7 +182,6 @@ export function mark_current_list_as_read(options) {
export function mark_stream_as_read(stream_id, cont) { export function mark_stream_as_read(stream_id, cont) {
channel.post({ channel.post({
url: "/json/mark_stream_as_read", url: "/json/mark_stream_as_read",
idempotent: true,
data: {stream_id}, data: {stream_id},
success: cont, success: cont,
}); });
@@ -192,7 +190,6 @@ export function mark_stream_as_read(stream_id, cont) {
export function mark_topic_as_read(stream_id, topic, cont) { export function mark_topic_as_read(stream_id, topic, cont) {
channel.post({ channel.post({
url: "/json/mark_topic_as_read", url: "/json/mark_topic_as_read",
idempotent: true,
data: {stream_id, topic_name: topic}, data: {stream_id, topic_name: topic},
success: cont, success: cont,
}); });

View File

@@ -17,7 +17,6 @@ export function server_update(opts) {
emoji_code: opts.emoji_code, emoji_code: opts.emoji_code,
reaction_type: opts.reaction_type, reaction_type: opts.reaction_type,
}, },
idempotent: true,
success() { success() {
if (opts.success) { if (opts.success) {
opts.success(); opts.success();