Auth hook #8

Closed
wants to merge 23 commits into
from

Conversation

Projects
None yet
3 participants
@justinfx
Contributor

justinfx commented Aug 26, 2011

Extracted authorization hook from larger pull request

justinfx and others added some commits Jul 29, 2011

connection.go: seperating general messages from heartbeats into diffe…
…rent channels.

heartbeat is delayed if the last message received is within the timeframe of the HeartbeatInterval.
added SetAuthorization/IsAuthorized to SocketIO, to allow user to spe…
…cify a custom

authorization hook using the original http request.
fixed a bug in the new delayed heartbeats code where poor timing woul…
…d cause a

heartbeat to be skipped occasionally. new method uses a DelayTimer that resets
every time a message is received from the client.
connection.go: using inconsistent heartbeat intervals apparently brea…
…ks socket.io 0.6 client, which also has its own hardcoded timer and expects them to keep coming consistently.
message.go: added a Bytes() method
codec_sio.go: defined Bytes() method; fixed typo in case expression
@madari

This comment has been minimized.

Show comment Hide comment
@madari

madari Aug 26, 2011

This is wrong. The idea is that we incrementally give more data to the decoder until it can decode a message.
A message can span over multiple Read():s (see reader() and ReadBufferSize). The decoder is responsible
for eating the buffer and connection.go is responsible for giving it data to crunch.

For example, using a 16-byte read buffer and sending the alphabets results in the following:

2011/08/26 15:38:50 sio/conn: read nr=16 err=
2011/08/26 15:38:50 sio/conn: read data=m52mABCDEFGH # this is appended to the decoder buffer and a Decode() is tried
2011/08/26 15:38:50 sio/conn: read nr=16 err=
2011/08/26 15:38:50 sio/conn: read data=IJKLMNOPQRSTUVWX # this is appended to the decoder buffer and a Decode() is tried
2011/08/26 15:38:50 sio/conn: read nr=16 err=
2011/08/26 15:38:50 sio/conn: read data=YZABCDEFGHIJKLMN # this is appended to the decoder buffer and a Decode() is tried
2011/08/26 15:38:50 sio/conn: read nr=12 err=
2011/08/26 15:38:50 sio/conn: read data=OPQRSTUVWXYZ # this is appended to the decoder buffer and a Decode() is tried
2011/08/26 15:38:50 Server received ABCDEFGHIJKLMNOPQRSTUVWXYZABCDEFGHIJKLMNOPQRSTUVWXYZ # Finally the decoder returns a decoded message

madari commented on 41ce614 Aug 26, 2011

This is wrong. The idea is that we incrementally give more data to the decoder until it can decode a message.
A message can span over multiple Read():s (see reader() and ReadBufferSize). The decoder is responsible
for eating the buffer and connection.go is responsible for giving it data to crunch.

For example, using a 16-byte read buffer and sending the alphabets results in the following:

2011/08/26 15:38:50 sio/conn: read nr=16 err=
2011/08/26 15:38:50 sio/conn: read data=m52mABCDEFGH # this is appended to the decoder buffer and a Decode() is tried
2011/08/26 15:38:50 sio/conn: read nr=16 err=
2011/08/26 15:38:50 sio/conn: read data=IJKLMNOPQRSTUVWX # this is appended to the decoder buffer and a Decode() is tried
2011/08/26 15:38:50 sio/conn: read nr=16 err=
2011/08/26 15:38:50 sio/conn: read data=YZABCDEFGHIJKLMN # this is appended to the decoder buffer and a Decode() is tried
2011/08/26 15:38:50 sio/conn: read nr=12 err=
2011/08/26 15:38:50 sio/conn: read data=OPQRSTUVWXYZ # this is appended to the decoder buffer and a Decode() is tried
2011/08/26 15:38:50 Server received ABCDEFGHIJKLMNOPQRSTUVWXYZABCDEFGHIJKLMNOPQRSTUVWXYZ # Finally the decoder returns a decoded message

This comment has been minimized.

Show comment Hide comment
@justinfx

justinfx Aug 26, 2011

Owner

Is this only for the codec_streaming?
It seems to me that ReadBufferSize has nothing to do with incremental reading. It just sets a size for socket.Read() and can cause a buffer full if its smaller than the message being sent. I must be missing something, but it looks like reader() will do a socket.Read(buf), and if it didnt get an error and did read any bytes it will just call c.receive(), which then puts that data into the decBuf and called Decode() right away. At that point its either looking to get back messages or an error. If its only a partial single message, I dont understand how that goes back through the logic of building up more data. receive() looks to be wanting a parsed message or error.

  • reader() *
    calls socket.Read(buf)
    if problem: die
    else if good data -> c.receive() (blocking)
    reader() is blocked
  • receive() *
    put data in dec buffer
    call dec.Decode() (blocking call)
    receive() is blocked
  • Decode() *
    Goes into a loop reading the buffer, but would never receive any new data from reader() because its blocking right now
    Reads everything it has builds and returns the message or an error
  • receive() *
    return from Decode()
    got a message? pass it to the handler
    error? return
  • reader() *
    return from receive()
    loop again and try to Read again.

Is what I am missing, that Decode() will return an error if it only got part of a message, to allow reader() to send more data through the process until it gets the full message, and THEN returns a msg and error nil?

Owner

justinfx replied Aug 26, 2011

Is this only for the codec_streaming?
It seems to me that ReadBufferSize has nothing to do with incremental reading. It just sets a size for socket.Read() and can cause a buffer full if its smaller than the message being sent. I must be missing something, but it looks like reader() will do a socket.Read(buf), and if it didnt get an error and did read any bytes it will just call c.receive(), which then puts that data into the decBuf and called Decode() right away. At that point its either looking to get back messages or an error. If its only a partial single message, I dont understand how that goes back through the logic of building up more data. receive() looks to be wanting a parsed message or error.

  • reader() *
    calls socket.Read(buf)
    if problem: die
    else if good data -> c.receive() (blocking)
    reader() is blocked
  • receive() *
    put data in dec buffer
    call dec.Decode() (blocking call)
    receive() is blocked
  • Decode() *
    Goes into a loop reading the buffer, but would never receive any new data from reader() because its blocking right now
    Reads everything it has builds and returns the message or an error
  • receive() *
    return from Decode()
    got a message? pass it to the handler
    error? return
  • reader() *
    return from receive()
    loop again and try to Read again.

Is what I am missing, that Decode() will return an error if it only got part of a message, to allow reader() to send more data through the process until it gets the full message, and THEN returns a msg and error nil?

This comment has been minimized.

Show comment Hide comment
@madari

madari Aug 26, 2011

ReadBufferSize determines how many bytes to read at most during a single read from a socket. Smaller buffer => more reads.
The size of the buffer does not affect in any other way. It does not e.g. limit the maximum size of messages.

Your analysis was almost right.
If Decode was called with incomplete data (meaning that no error was detected, but the data was
just not fully in the buffer yet) it will return 0 messages and no errors. It just means "nothing to see here". The Decoder keeps its own internal
state and knows where to continue when it's called the next time.

The idea is that this works:

// initialization
buf := new(bytes.Buffer)
dec := codec.NewDecoder(buf)

// write the first part of the message: the header (simulating partial read 1/2)
buf.WriteString("~m~3~m~")
messages, err := dec.Decode() // messages = [], err = nil

// write the rest of the message: the payload (simulating partial read 2/2)
buf.WriteString("123")
messages, err := dec.Decode() // messages = ["123"], err = nil

Additionally you can check out TestDecodeStreaming in codec_sio_test.go.
I hope this helps.

ReadBufferSize determines how many bytes to read at most during a single read from a socket. Smaller buffer => more reads.
The size of the buffer does not affect in any other way. It does not e.g. limit the maximum size of messages.

Your analysis was almost right.
If Decode was called with incomplete data (meaning that no error was detected, but the data was
just not fully in the buffer yet) it will return 0 messages and no errors. It just means "nothing to see here". The Decoder keeps its own internal
state and knows where to continue when it's called the next time.

The idea is that this works:

// initialization
buf := new(bytes.Buffer)
dec := codec.NewDecoder(buf)

// write the first part of the message: the header (simulating partial read 1/2)
buf.WriteString("~m~3~m~")
messages, err := dec.Decode() // messages = [], err = nil

// write the rest of the message: the payload (simulating partial read 2/2)
buf.WriteString("123")
messages, err := dec.Decode() // messages = ["123"], err = nil

Additionally you can check out TestDecodeStreaming in codec_sio_test.go.
I hope this helps.

This comment has been minimized.

Show comment Hide comment
@justinfx

justinfx Aug 26, 2011

Owner
Owner

justinfx replied Aug 26, 2011

This comment has been minimized.

Show comment Hide comment
@edsrzf

edsrzf Aug 26, 2011

I admit, I really don't have much of an idea how this code works or how it's supposed to work.

As far as I could tell this buffer just kept being written to without ever being reset, but apparently that's not the case?

I admit, I really don't have much of an idea how this code works or how it's supposed to work.

As far as I could tell this buffer just kept being written to without ever being reset, but apparently that's not the case?

@madari

This comment has been minimized.

Show comment Hide comment
@madari

madari Aug 26, 2011

if !sio.isAuthorized(req) {

if !sio.isAuthorized(req) {

@justinfx

This comment has been minimized.

Show comment Hide comment
@justinfx

justinfx Aug 26, 2011

Owner

Will update that.

Owner

justinfx commented on 538c292 Aug 26, 2011

Will update that.

@madari

This comment has been minimized.

Show comment Hide comment
@madari

madari Aug 26, 2011

Owner

I took the liberty to cherry-pick the relevant commit (the last commit) into the master.
Landed in commit b926021.
Thank you!

Owner

madari commented Aug 26, 2011

I took the liberty to cherry-pick the relevant commit (the last commit) into the master.
Landed in commit b926021.
Thank you!

@madari madari closed this Aug 26, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment