diff --git a/spdy-core/src/main/java/org/eclipse/jetty/spdy/api/Settings.java b/spdy-core/src/main/java/org/eclipse/jetty/spdy/api/Settings.java index 8f69a5021de..6db03e814c8 100644 --- a/spdy-core/src/main/java/org/eclipse/jetty/spdy/api/Settings.java +++ b/spdy-core/src/main/java/org/eclipse/jetty/spdy/api/Settings.java @@ -90,19 +90,22 @@ public class Settings implements Iterable return settings.toString(); } - public static enum ID + public static final class ID { - UPLOAD_BANDWIDTH(1), - DOWNLOAD_BANDWIDTH(2), - ROUND_TRIP_TIME(3), - MAX_CONCURRENT_STREAMS(4), - CURRENT_CONGESTION_WINDOW(5), - DOWNLOAD_RETRANSMISSION_RATE(6), - INITIAL_WINDOW_SIZE(7); + public static ID UPLOAD_BANDWIDTH = new ID(1); + public static ID DOWNLOAD_BANDWIDTH = new ID(2); + public static ID ROUND_TRIP_TIME = new ID(3); + public static ID MAX_CONCURRENT_STREAMS = new ID(4); + public static ID CURRENT_CONGESTION_WINDOW = new ID(5); + public static ID DOWNLOAD_RETRANSMISSION_RATE = new ID(6); + public static ID INITIAL_WINDOW_SIZE = new ID(7); - public static ID from(int code) + public synchronized static ID from(int code) { - return Codes.codes.get(code); + ID id = Codes.codes.get(code); + if (id == null) + id = new ID(code); + return id; } private final int code; @@ -113,11 +116,17 @@ public class Settings implements Iterable Codes.codes.put(code, this); } - public int getCode() + public int code() { return code; } + @Override + public String toString() + { + return String.valueOf(code); + } + private static class Codes { private static final Map codes = new HashMap<>(); @@ -126,31 +135,31 @@ public class Settings implements Iterable public static enum Flag { - NONE(0), - PERSIST(1), - PERSISTED(2); + NONE((byte)0), + PERSIST((byte)1), + PERSISTED((byte)2); - public static Flag from(int code) + public static Flag from(byte code) { return Codes.codes.get(code); } - private final int code; + private final byte code; - private Flag(int code) + private Flag(byte code) { this.code = code; Codes.codes.put(code, this); } - public int getCode() + public byte code() { return code; } private static class Codes { - private static final Map codes = new HashMap<>(); + private static final Map codes = new HashMap<>(); } } diff --git a/spdy-core/src/main/java/org/eclipse/jetty/spdy/generator/SettingsGenerator.java b/spdy-core/src/main/java/org/eclipse/jetty/spdy/generator/SettingsGenerator.java index 1c76be325d2..b792d44d074 100644 --- a/spdy-core/src/main/java/org/eclipse/jetty/spdy/generator/SettingsGenerator.java +++ b/spdy-core/src/main/java/org/eclipse/jetty/spdy/generator/SettingsGenerator.java @@ -41,10 +41,9 @@ public class SettingsGenerator extends ControlFrameGenerator for (Settings.Setting setting : settings) { - int id = setting.id().getCode(); - int flags = setting.flag().getCode(); - int idAndFlags = (id << 8) + flags; - idAndFlags = convertIdAndFlags(frame.getVersion(), idAndFlags); + int id = setting.id().code(); + byte flags = setting.flag().code(); + int idAndFlags = convertIdAndFlags(frame.getVersion(), id, flags); buffer.putInt(idAndFlags); buffer.putInt(setting.value()); } @@ -53,14 +52,16 @@ public class SettingsGenerator extends ControlFrameGenerator return buffer; } - private int convertIdAndFlags(short version, int idAndFlags) + private int convertIdAndFlags(short version, int id, byte flags) { switch (version) { case SPDY.V2: { - // A bug in the Chromium implementation made v2 have - // 3 bytes little endian + 1 byte of flags + // In v2 the format is 24 bits of ID + 8 bits of flag + int idAndFlags = (id << 8) + flags; + // A bug in the Chromium implementation forces v2 to have + // the 3 ID bytes little endian, so we swap first and third int result = idAndFlags & 0x00_FF_00_FF; result += (idAndFlags & 0xFF_00_00_00) >>> 16; result += (idAndFlags & 0x00_00_FF_00) << 16; @@ -68,7 +69,8 @@ public class SettingsGenerator extends ControlFrameGenerator } case SPDY.V3: { - return idAndFlags; + // In v3 the format is 8 bits of flags + 24 bits of ID + return (flags << 24) + (id & 0xFF_FF_FF); } default: { diff --git a/spdy-core/src/main/java/org/eclipse/jetty/spdy/parser/SettingsBodyParser.java b/spdy-core/src/main/java/org/eclipse/jetty/spdy/parser/SettingsBodyParser.java index 81903d88de7..d76382ad0e3 100644 --- a/spdy-core/src/main/java/org/eclipse/jetty/spdy/parser/SettingsBodyParser.java +++ b/spdy-core/src/main/java/org/eclipse/jetty/spdy/parser/SettingsBodyParser.java @@ -49,7 +49,7 @@ public class SettingsBodyParser extends ControlFrameBodyParser if (buffer.remaining() >= 4) { count = buffer.getInt(); - state = State.KEY; + state = State.ID_FLAGS; } else { @@ -64,10 +64,10 @@ public class SettingsBodyParser extends ControlFrameBodyParser --cursor; count += (currByte & 0xFF) << 8 * cursor; if (cursor == 0) - state = State.KEY; + state = State.ID_FLAGS; break; } - case KEY: + case ID_FLAGS: { if (buffer.remaining() >= 4) { @@ -76,12 +76,12 @@ public class SettingsBodyParser extends ControlFrameBodyParser } else { - state = State.KEY_BYTES; + state = State.ID_FLAGS_BYTES; cursor = 4; } break; } - case KEY_BYTES: + case ID_FLAGS_BYTES: { byte currByte = buffer.get(); --cursor; @@ -136,12 +136,14 @@ public class SettingsBodyParser extends ControlFrameBodyParser { case SPDY.V2: { - // A bug in the Chromium implementation made v2 have - // 3 bytes little endian + 1 byte of flags - // Here we normalize this to conform with v3 - int result = idAndFlags & 0x00_FF_00_FF; - result += (idAndFlags & 0xFF_00_00_00) >>> 16; - result += (idAndFlags & 0x00_00_FF_00) << 16; + // A bug in the Chromium implementation forces v2 to have + // 3 ID bytes little endian + 1 byte of flags + // Here we normalize this to conform with v3, which is + // 1 bytes of flag + 3 ID bytes big endian + int result = (idAndFlags & 0x00_00_00_FF) << 24; + result += (idAndFlags & 0x00_00_FF_00) << 8; + result += (idAndFlags & 0x00_FF_00_00) >>> 8; + result += (idAndFlags & 0xFF_00_00_00) >>> 24; return result; } case SPDY.V3: @@ -157,10 +159,10 @@ public class SettingsBodyParser extends ControlFrameBodyParser private boolean onPair() { - int id = (idAndFlags & 0xFF_FF_FF_00) >>> 8; - int flags = idAndFlags & 0xFF; + int id = idAndFlags & 0x00_FF_FF_FF; + byte flags = (byte)((idAndFlags & 0xFF_00_00_00) >>> 24); settings.put(new Settings.Setting(Settings.ID.from(id), Settings.Flag.from(flags), value)); - state = State.KEY; + state = State.ID_FLAGS; idAndFlags = 0; value = 0; --count; @@ -191,6 +193,6 @@ public class SettingsBodyParser extends ControlFrameBodyParser private enum State { - COUNT, COUNT_BYTES, KEY, KEY_BYTES, VALUE, VALUE_BYTES + COUNT, COUNT_BYTES, ID_FLAGS, ID_FLAGS_BYTES, VALUE, VALUE_BYTES } } diff --git a/spdy-jetty/src/test/java/org/eclipse/jetty/spdy/SettingsTest.java b/spdy-jetty/src/test/java/org/eclipse/jetty/spdy/SettingsTest.java index 39fb9e6916b..9ac5f678c44 100644 --- a/spdy-jetty/src/test/java/org/eclipse/jetty/spdy/SettingsTest.java +++ b/spdy-jetty/src/test/java/org/eclipse/jetty/spdy/SettingsTest.java @@ -16,9 +16,13 @@ package org.eclipse.jetty.spdy; +import java.net.InetSocketAddress; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import org.eclipse.jetty.spdy.api.SPDY; import org.eclipse.jetty.spdy.api.Session; import org.eclipse.jetty.spdy.api.SessionFrameListener; import org.eclipse.jetty.spdy.api.Settings; @@ -37,6 +41,10 @@ public class SettingsTest extends AbstractTest settings.put(new Settings.Setting(Settings.ID.MAX_CONCURRENT_STREAMS, Settings.Flag.PERSIST, streamsValue)); int windowValue = 32768; settings.put(new Settings.Setting(Settings.ID.INITIAL_WINDOW_SIZE, windowValue)); + int newCode = 91; + Settings.ID newID = Settings.ID.from(newCode); + int newValue = 97; + settings.put(new Settings.Setting(newID, newValue)); Settings.Setting setting1 = settings.get(Settings.ID.MAX_CONCURRENT_STREAMS); Assert.assertSame(Settings.ID.MAX_CONCURRENT_STREAMS, setting1.id()); @@ -47,6 +55,13 @@ public class SettingsTest extends AbstractTest Assert.assertSame(Settings.ID.INITIAL_WINDOW_SIZE, setting2.id()); Assert.assertSame(Settings.Flag.NONE, setting2.flag()); Assert.assertEquals(windowValue, setting2.value()); + + int size = settings.size(); + Settings.Setting setting3 = settings.remove(Settings.ID.from(newCode)); + Assert.assertEquals(size - 1, settings.size()); + Assert.assertNotNull(setting3); + Assert.assertSame(newID, setting3.id()); + Assert.assertEquals(newValue, setting3.value()); } @Test @@ -108,4 +123,42 @@ public class SettingsTest extends AbstractTest Assert.assertTrue(latch.await(5, TimeUnit.SECONDS)); } + + @Test + public void testSettingIDIsTheSameInBothV2AndV3() throws Exception + { + final AtomicReference v2 = new AtomicReference<>(); + final AtomicReference v3 = new AtomicReference<>(); + final CountDownLatch settingsLatch = new CountDownLatch(2); + InetSocketAddress address = startServer(new ServerSessionFrameListener.Adapter() + { + private final AtomicInteger count = new AtomicInteger(); + + @Override + public void onSettings(Session session, SettingsInfo settingsInfo) + { + int count = this.count.incrementAndGet(); + if (count == 1) + v2.set(settingsInfo); + else if (count == 2) + v3.set(settingsInfo); + else + Assert.fail(); + settingsLatch.countDown(); + } + }); + + Settings settings = new Settings(); + settings.put(new Settings.Setting(Settings.ID.INITIAL_WINDOW_SIZE, Settings.Flag.PERSIST, 0xC0_00)); + SettingsInfo settingsInfo = new SettingsInfo(settings); + + Session sessionV2 = startClient(address, null); + sessionV2.settings(settingsInfo); + + Session sessionV3 = clientFactory.newSPDYClient(SPDY.V3).connect(address, null).get(5, TimeUnit.SECONDS); + sessionV3.settings(settingsInfo); + + Assert.assertTrue(settingsLatch.await(5, TimeUnit.SECONDS)); + Assert.assertEquals(v2.get().getSettings(), v3.get().getSettings()); + } }