From 3d1c4f2488fbfc55ffc95ea42d6fc35ec8cf27a5 Mon Sep 17 00:00:00 2001 From: Seth Call Date: Thu, 19 Jun 2014 14:05:33 -0500 Subject: [PATCH] * VRFS-1790 - drag handles added to configure tracks; VRFS-1653 - fixed bug where slow initial connect would cause loop; also log in on initail connect instead of extra login message --- admin/spec/factories.rb | 1 + db/manifest | 1 + db/up/connection_channel_id.sql | 1 + pb/src/client_container.proto | 7 + ruby/lib/jam_ruby/connection_manager.rb | 10 +- ruby/lib/jam_ruby/message_factory.rb | 11 ++ ruby/lib/jam_ruby/models/latency_tester.rb | 2 + ruby/spec/factories.rb | 1 + ruby/spec/jam_ruby/connection_manager_spec.rb | 42 +++--- .../jam_ruby/models/latency_tester_spec.rb | 2 +- .../assets/javascripts/AAB_message_factory.js | 3 +- web/app/assets/javascripts/JamServer.js | 25 ++-- .../javascripts/configureTracksHelper.js | 6 +- web/app/assets/javascripts/jamkazam.js | 15 +- .../client/dragDropTracks.css.scss | 16 ++- web/spec/factories.rb | 1 + .../lib/jam_websockets/router.rb | 136 +++++++++--------- websocket-gateway/spec/factories.rb | 1 + .../spec/jam_websockets/router_spec.rb | 30 ++-- 19 files changed, 182 insertions(+), 129 deletions(-) create mode 100644 db/up/connection_channel_id.sql diff --git a/admin/spec/factories.rb b/admin/spec/factories.rb index a2d058d80..93063a175 100644 --- a/admin/spec/factories.rb +++ b/admin/spec/factories.rb @@ -36,6 +36,7 @@ FactoryGirl.define do addr 0 locidispid 0 client_type 'client' + sequence(:channel_id) { |n| "Channel#{n}"} association :user, factory: :user end diff --git a/db/manifest b/db/manifest index 28ce6b527..7b5dd70bb 100755 --- a/db/manifest +++ b/db/manifest @@ -162,3 +162,4 @@ indexing_for_regions.sql latency_tester.sql fix_users_location_fields.sql audio_latency.sql +connection_channel_id.sql \ No newline at end of file diff --git a/db/up/connection_channel_id.sql b/db/up/connection_channel_id.sql new file mode 100644 index 000000000..4ea21524f --- /dev/null +++ b/db/up/connection_channel_id.sql @@ -0,0 +1 @@ +ALTER TABLE connections ADD COLUMN channel_id VARCHAR(256) NOT NULL; \ No newline at end of file diff --git a/pb/src/client_container.proto b/pb/src/client_container.proto index 276349154..876a0aeb3 100644 --- a/pb/src/client_container.proto +++ b/pb/src/client_container.proto @@ -77,6 +77,7 @@ message ClientMessage { SERVER_REJECTION_ERROR = 1005; SERVER_PERMISSION_ERROR = 1010; SERVER_BAD_STATE_ERROR = 1015; + SERVER_DUPLICATE_CLIENT_ERROR = 1016; } // Identifies which inner message is filled in @@ -159,6 +160,7 @@ message ClientMessage { optional ServerRejectionError server_rejection_error = 1005; optional ServerPermissionError server_permission_error = 1010; optional ServerBadStateError server_bad_state_error = 1015; + optional ServerDuplicateClientError server_duplicate_client_error = 1016; } // route_to: server @@ -529,4 +531,9 @@ message ServerBadStateError { optional string error_msg = 1; } +// route_to: client +// this indicates that we detected another client with the same client ID +message ServerDuplicateClientError { +} + diff --git a/ruby/lib/jam_ruby/connection_manager.rb b/ruby/lib/jam_ruby/connection_manager.rb index 4916da854..30d31c1eb 100644 --- a/ruby/lib/jam_ruby/connection_manager.rb +++ b/ruby/lib/jam_ruby/connection_manager.rb @@ -44,7 +44,7 @@ module JamRuby end # reclaim the existing connection, if ip_address is not nil then perhaps a new address as well - def reconnect(conn, reconnect_music_session_id, ip_address, connection_stale_time, connection_expire_time) + def reconnect(conn, channel_id, reconnect_music_session_id, ip_address, connection_stale_time, connection_expire_time) music_session_id = nil reconnected = false @@ -86,7 +86,7 @@ module JamRuby end sql =< ClientMessage::Type::SERVER_DUPLICATE_CLIENT_ERROR, + :route_to => CLIENT_TARGET, + :server_duplicate_client_error => error + ) + end + ###################################### NOTIFICATIONS ###################################### # create a friend update message diff --git a/ruby/lib/jam_ruby/models/latency_tester.rb b/ruby/lib/jam_ruby/models/latency_tester.rb index 2bc1071ae..e4e36170e 100644 --- a/ruby/lib/jam_ruby/models/latency_tester.rb +++ b/ruby/lib/jam_ruby/models/latency_tester.rb @@ -19,6 +19,7 @@ module JamRuby # or bootstrap a new latency_tester def self.connect(options) client_id = options[:client_id] + channel_id = options[:channel_id] ip_address = options[:ip_address] connection_stale_time = options[:connection_stale_time] connection_expire_time = options[:connection_expire_time] @@ -69,6 +70,7 @@ module JamRuby connection.stale_time = connection_stale_time connection.expire_time = connection_expire_time connection.as_musician = false + connection.channel_id = channel_id unless connection.save return connection end diff --git a/ruby/spec/factories.rb b/ruby/spec/factories.rb index b5f1d57df..0364fd74c 100644 --- a/ruby/spec/factories.rb +++ b/ruby/spec/factories.rb @@ -112,6 +112,7 @@ FactoryGirl.define do addr 0 locidispid 0 client_type 'client' + sequence(:channel_id) { |n| "Channel#{n}"} association :user, factory: :user end diff --git a/ruby/spec/jam_ruby/connection_manager_spec.rb b/ruby/spec/jam_ruby/connection_manager_spec.rb index bdb397262..d6976fe50 100644 --- a/ruby/spec/jam_ruby/connection_manager_spec.rb +++ b/ruby/spec/jam_ruby/connection_manager_spec.rb @@ -9,6 +9,8 @@ describe ConnectionManager do STALE_BUT_NOT_EXPIRED = 50 DEFINITELY_EXPIRED = 70 + let(:channel_id) {'1'} + before do @conn = PG::Connection.new(:dbname => SpecDb::TEST_DB_NAME, :user => "postgres", :password => "postgres", :host => "localhost") @connman = ConnectionManager.new(:conn => @conn) @@ -46,8 +48,8 @@ describe ConnectionManager do user.save! user = nil - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) - expect { @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) }.to raise_error(PG::Error) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + expect { @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) }.to raise_error(PG::Error) end it "create connection then delete it" do @@ -56,7 +58,7 @@ describe ConnectionManager do #user_id = create_user("test", "user2", "user2@jamkazam.com") user = FactoryGirl.create(:user) - count = @connman.create_connection(user.id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + count = @connman.create_connection(user.id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) count.should == 1 @@ -86,7 +88,7 @@ describe ConnectionManager do #user_id = create_user("test", "user2", "user2@jamkazam.com") user = FactoryGirl.create(:user) - count = @connman.create_connection(user.id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + count = @connman.create_connection(user.id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) count.should == 1 @@ -102,7 +104,7 @@ describe ConnectionManager do cc.addr.should == 0x01010101 cc.locidispid.should == 17192000002 - @connman.reconnect(cc, nil, "33.1.2.3", STALE_TIME, EXPIRE_TIME) + @connman.reconnect(cc, channel_id, nil, "33.1.2.3", STALE_TIME, EXPIRE_TIME) cc = Connection.find_by_client_id!(client_id) cc.connected?.should be_true @@ -215,7 +217,7 @@ describe ConnectionManager do it "flag stale connection" do client_id = "client_id8" user_id = create_user("test", "user8", "user8@jamkazam.com") - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) num = JamRuby::Connection.count(:conditions => ['aasm_state = ?','connected']) num.should == 1 @@ -256,7 +258,7 @@ describe ConnectionManager do it "expires stale connection" do client_id = "client_id8" user_id = create_user("test", "user8", "user8@jamkazam.com") - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) conn = Connection.find_by_client_id(client_id) set_updated_at(conn, Time.now - STALE_BUT_NOT_EXPIRED) @@ -282,7 +284,7 @@ describe ConnectionManager do music_session_id = music_session.id user = User.find(user_id) - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) connection = @connman.join_music_session(user, client_id, music_session, true, TRACKS, 10) connection.errors.any?.should be_false @@ -318,8 +320,8 @@ describe ConnectionManager do client_id2 = "client_id10.12" user_id = create_user("test", "user10.11", "user10.11@jamkazam.com", :musician => true) user_id2 = create_user("test", "user10.12", "user10.12@jamkazam.com", :musician => false) - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) - @connman.create_connection(user_id2, client_id2, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id2, client_id2, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) music_session = FactoryGirl.create(:active_music_session, user_id: user_id) music_session_id = music_session.id @@ -338,7 +340,7 @@ describe ConnectionManager do client_id = "client_id10.2" user_id = create_user("test", "user10.2", "user10.2@jamkazam.com") - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) music_session = FactoryGirl.create(:active_music_session, user_id: user_id) user = User.find(user_id) @@ -354,8 +356,8 @@ describe ConnectionManager do fan_client_id = "client_id10.4" musician_id = create_user("test", "user10.3", "user10.3@jamkazam.com") fan_id = create_user("test", "user10.4", "user10.4@jamkazam.com", :musician => false) - @connman.create_connection(musician_id, musician_client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) - @connman.create_connection(fan_id, fan_client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(musician_id, musician_client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(fan_id, fan_client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) music_session = FactoryGirl.create(:active_music_session, :fan_access => false, user_id: musician_id) music_session_id = music_session.id @@ -379,7 +381,7 @@ describe ConnectionManager do music_session_id = music_session.id user = User.find(user_id2) - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) # specify real user id, but not associated with this session expect { @connman.join_music_session(user, client_id, music_session, true, TRACKS, 10) } .to raise_error(ActiveRecord::RecordNotFound) end @@ -391,7 +393,7 @@ describe ConnectionManager do user = User.find(user_id) music_session = ActiveMusicSession.new - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) connection = @connman.join_music_session(user, client_id, music_session, true, TRACKS, 10) connection.errors.size.should == 1 connection.errors.get(:music_session).should == [ValidationMessages::MUSIC_SESSION_MUST_BE_SPECIFIED] @@ -405,7 +407,7 @@ describe ConnectionManager do music_session_id = music_session.id user = User.find(user_id2) - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) # specify real user id, but not associated with this session expect { @connman.join_music_session(user, client_id, music_session, true, TRACKS, 10) } .to raise_error(ActiveRecord::RecordNotFound) end @@ -419,7 +421,7 @@ describe ConnectionManager do user = User.find(user_id) dummy_music_session = ActiveMusicSession.new - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) expect { @connman.leave_music_session(user, Connection.find_by_client_id(client_id), dummy_music_session) }.to raise_error(JamRuby::StateError) end @@ -434,7 +436,7 @@ describe ConnectionManager do dummy_music_session = ActiveMusicSession.new - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) @connman.join_music_session(user, client_id, music_session, true, TRACKS, 10) expect { @connman.leave_music_session(user, Connection.find_by_client_id(client_id), dummy_music_session) }.to raise_error(JamRuby::StateError) end @@ -447,7 +449,7 @@ describe ConnectionManager do music_session_id = music_session.id user = User.find(user_id) - @connman.create_connection(user_id, client_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) @connman.join_music_session(user, client_id, music_session, true, TRACKS, 10) assert_session_exists(music_session_id, true) @@ -490,7 +492,7 @@ describe ConnectionManager do user = User.find(user_id) client_id1 = Faker::Number.number(20) - @connman.create_connection(user_id, client_id1, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) + @connman.create_connection(user_id, client_id1, channel_id, "1.1.1.1", 'client', STALE_TIME, EXPIRE_TIME) music_session1 = FactoryGirl.create(:active_music_session, :user_id => user_id) connection1 = @connman.join_music_session(user, client_id1, music_session1, true, TRACKS, 10) connection1.errors.size.should == 0 diff --git a/ruby/spec/jam_ruby/models/latency_tester_spec.rb b/ruby/spec/jam_ruby/models/latency_tester_spec.rb index e0d3071d2..4bae7633d 100644 --- a/ruby/spec/jam_ruby/models/latency_tester_spec.rb +++ b/ruby/spec/jam_ruby/models/latency_tester_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe LatencyTester do - let(:params) {{client_id: 'abc', ip_address: '10.1.1.1', connection_stale_time:40, connection_expire_time:60} } + let(:params) {{client_id: 'abc', ip_address: '10.1.1.1', connection_stale_time:40, connection_expire_time:60, channel_id: '1'} } it "success" do latency_tester = FactoryGirl.create(:latency_tester) diff --git a/web/app/assets/javascripts/AAB_message_factory.js b/web/app/assets/javascripts/AAB_message_factory.js index 424754227..1957220a7 100644 --- a/web/app/assets/javascripts/AAB_message_factory.js +++ b/web/app/assets/javascripts/AAB_message_factory.js @@ -66,7 +66,8 @@ SERVER_BAD_STATE_RECOVERED: "SERVER_BAD_STATE_RECOVERED", SERVER_GENERIC_ERROR : "SERVER_GENERIC_ERROR", SERVER_REJECTION_ERROR : "SERVER_REJECTION_ERROR", - SERVER_BAD_STATE_ERROR : "SERVER_BAD_STATE_ERROR" + SERVER_BAD_STATE_ERROR : "SERVER_BAD_STATE_ERROR", + SERVER_DUPLICATE_CLIENT_ERROR : "SERVER_DUPLICATE_CLIENT_ERROR" }; var route_to = context.JK.RouteToPrefix = { diff --git a/web/app/assets/javascripts/JamServer.js b/web/app/assets/javascripts/JamServer.js index 00771404a..def88672f 100644 --- a/web/app/assets/javascripts/JamServer.js +++ b/web/app/assets/javascripts/JamServer.js @@ -99,7 +99,7 @@ heartbeatAckCheckInterval = null; } - if (!server.reconnecting) { + if (!server.reconnecting && !server.noReconnect) { server.reconnecting = true; var result = activeElementEvent('beforeDisconnect'); @@ -170,6 +170,8 @@ function loggedIn(header, payload) { + server.reconnecting = false; + if (!connectTimeout) { clearTimeout(connectTimeout); connectTimeout = null; @@ -271,7 +273,6 @@ else { window.location.reload(); } - server.reconnecting = false; }); @@ -453,7 +454,18 @@ connectDeferred = new $.Deferred(); channelId = context.JK.generateUUID(); // create a new channel ID for every websocket connection - var uri = context.gon.websocket_gateway_uri + '?channel_id=' + channelId; // Set in index.html.erb. + // we will log in one of 3 ways: + // browser: use session cookie, and auth with token + // native: use session cookie, and use the token + // latency_tester: ask for client ID from backend; no token (trusted) + var params = { + channel_id: channelId, + token: $.cookie("remember_token"), + client_type: isClientMode() ? context.JK.clientType : 'latency_tester', + client_id: isClientMode() ? (gon.global.env == "development" ? $.cookie('client_id') : null): context.jamClient.clientID + } + + var uri = context.gon.websocket_gateway_uri + '?' + $.param(params); // Set in index.html.erb. logger.debug("connecting websocket: " + uri); @@ -499,12 +511,7 @@ server.onOpen = function () { logger.debug("server.onOpen"); - if(isClientMode()) { - server.rememberLogin(); - } - else { - server.latencyTesterLogin(); - } + // we should receive LOGIN_ACK very soon. we already set a timer elsewhere to give 4 seconds to receive it }; server.onMessage = function (e) { diff --git a/web/app/assets/javascripts/configureTracksHelper.js b/web/app/assets/javascripts/configureTracksHelper.js index 90a3fb510..e2beb0656 100644 --- a/web/app/assets/javascripts/configureTracksHelper.js +++ b/web/app/assets/javascripts/configureTracksHelper.js @@ -67,7 +67,9 @@ .css('min-width', $channel.width()) .css('min-height', $channel.height()) .css('z-index', 10000) - .data('original', $channel) + .css('background-position', '4px 6px') + .css('padding-left', '14px') + .data('original', $channel); $channel.data('cloned', hoverChannel); hoverChannel @@ -120,6 +122,8 @@ .css('width', '') .css('background-color', '') .css('padding', '') + .css('padding-left', '') + .css('background-position', '') .css('border', '') .css('border-radius', '') .css('right', '') diff --git a/web/app/assets/javascripts/jamkazam.js b/web/app/assets/javascripts/jamkazam.js index 4891c3509..390beca45 100644 --- a/web/app/assets/javascripts/jamkazam.js +++ b/web/app/assets/javascripts/jamkazam.js @@ -94,22 +94,30 @@ context.jamClient.OnDownloadAvailable(); } + /** + * This most likely occurs when multiple tabs in the same browser are open, until we make a fix for this... + */ + function duplicateClientError() { + context.JK.Banner.showAlert("Duplicate Window (Development Mode Only)", "You have logged in another window in this browser. This window will continue to work but with degraded functionality. This limitation will soon be fixed.") + context.JK.JamServer.noReconnect = true; + } function registerBadStateError() { - logger.debug("register for server_bad_state_error"); context.JK.JamServer.registerMessageCallback(context.JK.MessageType.SERVER_BAD_STATE_ERROR, serverBadStateError); } function registerBadStateRecovered() { - logger.debug("register for server_bad_state_recovered"); context.JK.JamServer.registerMessageCallback(context.JK.MessageType.SERVER_BAD_STATE_RECOVERED, serverBadStateRecovered); } function registerDownloadAvailable() { - logger.debug("register for download_available"); context.JK.JamServer.registerMessageCallback(context.JK.MessageType.DOWNLOAD_AVAILABLE, downloadAvailable); } + function registerDuplicateClientError() { + context.JK.JamServer.registerMessageCallback(context.JK.MessageType.SERVER_DUPLICATE_CLIENT_ERROR, duplicateClientError); + } + /** * Generic error handler for Ajax calls. */ @@ -330,6 +338,7 @@ registerBadStateRecovered(); registerBadStateError(); registerDownloadAvailable(); + registerDuplicateClientError(); context.JK.FaderHelpers.initialize(); context.window.onunload = this.unloadFunction; diff --git a/web/app/assets/stylesheets/client/dragDropTracks.css.scss b/web/app/assets/stylesheets/client/dragDropTracks.css.scss index bc497cc05..986f97236 100644 --- a/web/app/assets/stylesheets/client/dragDropTracks.css.scss +++ b/web/app/assets/stylesheets/client/dragDropTracks.css.scss @@ -27,6 +27,12 @@ } } + .channels-holder { + .ftue-input { + background-position:4px 6px; + padding-left:14px; + } + } .unassigned-input-channels { min-height: 240px; overflow-y: auto; @@ -39,6 +45,7 @@ } } .ftue-input { + } &.drag-in-progress { @@ -84,6 +91,7 @@ font-size: 12px; cursor: move; padding: 4px; + padding-left:10px; border: solid 1px #999; margin-bottom: 15px; white-space: nowrap; @@ -91,6 +99,9 @@ text-overflow: ellipsis; text-align: left; line-height:20px; + background-image:url('/assets/content/icon_drag_handle.png'); + background-position:0 3px; + background-repeat:no-repeat; //direction: rtl; &.ui-draggable-dragging { margin-bottom: 0; @@ -126,10 +137,13 @@ .ftue-input { padding: 0; + padding-left:10px; border: 0; margin-bottom: 0; &.ui-draggable-dragging { padding: 4px; + padding-left:14px; + background-position:4px 6px; border: solid 1px #999; overflow: visible; } @@ -163,7 +177,7 @@ }*/ } &:nth-of-type(2) { - padding-left:2px; + padding-left:10px; float: right; } } diff --git a/web/spec/factories.rb b/web/spec/factories.rb index f3bdd0ed4..76d05e751 100644 --- a/web/spec/factories.rb +++ b/web/spec/factories.rb @@ -130,6 +130,7 @@ FactoryGirl.define do addr 0 locidispid 0 client_type 'client' + sequence(:channel_id) { |n| "Channel#{n}"} end factory :friendship, :class => JamRuby::Friendship do diff --git a/websocket-gateway/lib/jam_websockets/router.rb b/websocket-gateway/lib/jam_websockets/router.rb index 3caee5042..f39852132 100644 --- a/websocket-gateway/lib/jam_websockets/router.rb +++ b/websocket-gateway/lib/jam_websockets/router.rb @@ -11,22 +11,6 @@ module EventMachine module WebSocket class Connection < EventMachine::Connection attr_accessor :encode_json, :channel_id, :client_id, :user_id, :context, :trusted # client_id is uuid we give to each client to track them as we like - - # http://stackoverflow.com/questions/11150147/how-to-check-if-eventmachineconnection-is-open - attr_accessor :connected - def connection_completed - connected = true - super - end - - def connected? - !!connected - end - - def unbind - connected = false - super - end end end end @@ -226,6 +210,40 @@ module JamWebsockets MQRouter.client_exchange = @clients_exchange end + # this method allows you to translate exceptions into websocket channel messages and behavior safely. + # pass in your block, throw an error in your logic, and have the right things happen on the websocket channel + def websocket_comm(client, original_message_id, &blk) + begin + blk.call + rescue SessionError => e + @log.info "ending client session deliberately due to malformed client behavior. reason=#{e}" + begin + # wrap the message up and send it down + error_msg = @message_factory.server_rejection_error(e.to_s) + send_to_client(client, error_msg) + ensure + cleanup_client(client) + end + rescue PermissionError => e + @log.info "permission error. reason=#{e.to_s}" + @log.info e + + # wrap the message up and send it down + error_msg = @message_factory.server_permission_error(original_message_id, e.to_s) + send_to_client(client, error_msg) + rescue => e + @log.error "ending client session due to server programming or runtime error. reason=#{e.to_s}" + @log.error e + + begin + # wrap the message up and send it down + error_msg = @message_factory.server_generic_error(e.to_s) + send_to_client(client, error_msg) + ensure + cleanup_client(client) + end + end + end def new_client(client, is_trusted) # default to using json instead of pb @@ -246,6 +264,9 @@ module JamWebsockets client.encode_json = false end + websocket_comm(client, nil) do + handle_login(client, handshake.query) + end } client.onclose { @@ -261,50 +282,26 @@ module JamWebsockets end } - client.onmessage { |msg| + client.onmessage { |data| # TODO: set a max message size before we put it through PB? # TODO: rate limit? - pb_msg = nil + msg = nil - begin + # extract the message safely + websocket_comm(client, nil) do if client.encode_json - #example: {"type":"LOGIN", "target":"server", "login" : {"username":"hi"}} - parse = JSON.parse(msg) - pb_msg = Jampb::ClientMessage.json_create(parse) - self.route(pb_msg, client) + json = JSON.parse(data) + msg = Jampb::ClientMessage.json_create(json) else - pb_msg = Jampb::ClientMessage.parse(msg.to_s) - self.route(pb_msg, client) + msg = Jampb::ClientMessage.parse(data.to_s) end - rescue SessionError => e - @log.info "ending client session deliberately due to malformed client behavior. reason=#{e}" - begin - # wrap the message up and send it down - error_msg = @message_factory.server_rejection_error(e.to_s) - send_to_client(client, error_msg) - ensure - cleanup_client(client) - end - rescue PermissionError => e - @log.info "permission error. reason=#{e.to_s}" - @log.info e + end - # wrap the message up and send it down - error_msg = @message_factory.server_permission_error(pb_msg.message_id, e.to_s) - send_to_client(client, error_msg) - rescue => e - @log.error "ending client session due to server programming or runtime error. reason=#{e.to_s}" - @log.error e - - begin - # wrap the message up and send it down - error_msg = @message_factory.server_generic_error(e.to_s) - send_to_client(client, error_msg) - ensure - cleanup_client(client) - end + # then route it internally + websocket_comm(client, msg.message_id) do + self.route(msg, client) end } end @@ -391,9 +388,9 @@ module JamWebsockets # removes all resources associated with a client def cleanup_client(client) - @semaphore.synchronize do - client.close if client.connected? + client.close + @semaphore.synchronize do pending = client.context.nil? # presence of context implies this connection has been logged into if pending @@ -514,6 +511,7 @@ module JamWebsockets heartbeat_interval, connection_stale_time, connection_expire_time = determine_connection_times(nil, client_type) latency_tester = LatencyTester.connect({ client_id: client_id, + channel_id: client.channel_id, ip_address: remote_ip, connection_stale_time: connection_stale_time, connection_expire_time: connection_expire_time}) @@ -543,15 +541,15 @@ module JamWebsockets end end - def handle_login(login, client) - username = login.username if login.value_for_tag(1) - password = login.password if login.value_for_tag(2) - token = login.token if login.value_for_tag(3) - client_id = login.client_id if login.value_for_tag(4) - reconnect_music_session_id = login.reconnect_music_session_id if login.value_for_tag(5) - client_type = login.client_type if login.value_for_tag(6) + def handle_login(client, options) + username = options["username"] + password = options["password"] + token = options["token"] + client_id = options["client_id"] + reconnect_music_session_id = options["music_session_id"] + client_type = options["client_type"] - @log.info("*** handle_login: token=#{token}; client_id=#{client_id}, client_type=#{client_type}") + @log.info("handle_login: client_type=#{client_type} token=#{token} client_id=#{client_id} channel_id=#{client.channel_id}") if client_type == Connection::TYPE_LATENCY_TESTER handle_latency_tester_login(client_id, client_type, client) @@ -568,17 +566,21 @@ module JamWebsockets user = valid_login(username, password, token, client_id) + # XXX This logic needs to instead be handled by a broadcast out to all websockets indicating dup # kill any websocket connections that have this same client_id, which can happen in race conditions # this code must happen here, before we go any further, so that there is only one websocket connection per client_id existing_context = @client_lookup[client_id] if existing_context - # in some reconnect scenarios, we may have in memory a websocket client still. + # in some reconnect scenarios, we may have in memory a websocket client still. + # let's whack it, and tell the other client, if still connected, that this is a duplicate login attempt @log.info "duplicate client: #{existing_context}" - Diagnostic.duplicate_client(existing_context.user, existing_context) if existing_context.client.connected + Diagnostic.duplicate_client(existing_context.user, existing_context) + error_msg = @message_factory.server_duplicate_client_error + send_to_client(existing_context.client, error_msg) cleanup_client(existing_context.client) end - connection = JamRuby::Connection.find_by_client_id(client_id) + connection = Connection.find_by_client_id(client_id) # if this connection is reused by a different user (possible in logout/login scenarios), then whack the connection # because it will recreate a new connection lower down if connection && user && connection.user != user @@ -608,7 +610,7 @@ module JamWebsockets recording_id = nil ConnectionManager.active_record_transaction do |connection_manager| - music_session_id, reconnected = connection_manager.reconnect(connection, reconnect_music_session_id, remote_ip, connection_stale_time, connection_expire_time) + music_session_id, reconnected = connection_manager.reconnect(connection, client.channel_id, reconnect_music_session_id, remote_ip, connection_stale_time, connection_expire_time) if music_session_id.nil? # if this is a reclaim of a connection, but music_session_id comes back null, then we need to check if this connection was IN a music session before. @@ -644,7 +646,7 @@ module JamWebsockets unless connection # log this connection in the database ConnectionManager.active_record_transaction do |connection_manager| - connection_manager.create_connection(user.id, client.client_id, remote_ip, client_type, connection_stale_time, connection_expire_time) do |conn, count| + connection_manager.create_connection(user.id, client.client_id, client.channel_id, remote_ip, client_type, connection_stale_time, connection_expire_time) do |conn, count| if count == 1 Notification.send_friend_update(user.id, true, conn) end @@ -755,7 +757,7 @@ module JamWebsockets if !token.nil? && token != '' @log.debug "logging in via token" # attempt login with token - user = JamRuby::User.find_by_remember_token(token) + user = User.find_by_remember_token(token) if user.nil? @log.debug "no user found with token #{token}" diff --git a/websocket-gateway/spec/factories.rb b/websocket-gateway/spec/factories.rb index cf4fe8586..5b37ae1c4 100644 --- a/websocket-gateway/spec/factories.rb +++ b/websocket-gateway/spec/factories.rb @@ -87,6 +87,7 @@ FactoryGirl.define do ip_address '1.1.1.1' as_musician true client_type 'client' + sequence(:channel_id) { |n| "Channel#{n}"} end factory :instrument, :class => JamRuby::Instrument do diff --git a/websocket-gateway/spec/jam_websockets/router_spec.rb b/websocket-gateway/spec/jam_websockets/router_spec.rb index 6d1743265..31020c738 100644 --- a/websocket-gateway/spec/jam_websockets/router_spec.rb +++ b/websocket-gateway/spec/jam_websockets/router_spec.rb @@ -41,7 +41,7 @@ end # does a login and returns client -def login(router, user, password, client_id) +def login(router, user, password, client_id, token, client_type) message_factory = MessageFactory.new client = LoginClient.new @@ -60,15 +60,9 @@ def login(router, user, password, client_id) @router.new_client(client, false) handshake = double("handshake") - handshake.should_receive(:query).twice.and_return({ "pb" => "true", "channel_id" => SecureRandom.uuid }) + handshake.should_receive(:query).exactly(3).times.and_return({ "pb" => "true", "channel_id" => SecureRandom.uuid, "client_id" => client_id, "token" => token, "client_type" => client_type }) client.onopenblock.call handshake - # create a login message, and pass it into the router via onmsgblock.call - login = message_factory.login_with_user_pass(user.email, password, :client_id => client_id, :client_type => 'client') - - # first log in - client.onmsgblock.call login.to_s - client end @@ -98,15 +92,9 @@ def login_latency_tester(router, latency_tester, client_id) @router.new_client(client, true) handshake = double("handshake") - handshake.should_receive(:query).twice.and_return({ "pb" => "true", "channel_id" => SecureRandom.uuid }) + handshake.should_receive(:query).exactly(3).times.and_return({ "pb" => "true", "channel_id" => SecureRandom.uuid, "client_type" => "latency_tester", "client_id" => client_id }) client.onopenblock.call handshake - # create a login message, and pass it into the router via onmsgblock.call - login = message_factory.login_with_client_id(client_id) - - # first log in - client.onmsgblock.call login.to_s - client end @@ -239,7 +227,7 @@ describe Router do it "should allow login of valid user", :mq => true do @user = FactoryGirl.create(:user, :password => "foobar", :password_confirmation => "foobar") - client1 = login(@router, @user, "foobar", "1") + client1 = login(@router, @user, "foobar", "1", @user.remember_token, "client") done end @@ -271,7 +259,7 @@ describe Router do # make a music_session and define two members # create client 1, log him in, and log him in to music session - client1 = login(@router, user1, "foobar", "1") + client1 = login(@router, user1, "foobar", "1", user1.remember_token, "client") done end @@ -284,9 +272,9 @@ describe Router do # create client 1, log him in, and log him in to music session - client1 = login(@router, user1, "foobar", "1") + client1 = login(@router, user1, "foobar", "1", user1.remember_token, "client") - client2 = login(@router, user2, "foobar", "2") + client2 = login(@router, user2, "foobar", "2", user2.remember_token, "client") # make a music_session and define two members @@ -305,10 +293,10 @@ describe Router do music_session = FactoryGirl.create(:active_music_session, :creator => user1) # create client 1, log him in, and log him in to music session - client1 = login(@router, user1, "foobar", "1") + client1 = login(@router, user1, "foobar", "1", user1.remember_token, "client") #login_music_session(@router, client1, music_session) - client2 = login(@router, user2, "foobar", "2") + client2 = login(@router, user2, "foobar", "2", user2.remember_token, "client") #login_music_session(@router, client2, music_session) # by creating