[PATCH 0/9] http fixes

0 views
Skip to first unread message

Felipe Contreras

unread,
Feb 10, 2010, 3:03:28 PM2/10/10
to msn-...@googlegroups.com, Felipe Contreras
From: Felipe Contreras <felipe.c...@nokia.com>

Hi,

I configured an HTTP proxy and noticed many issues with the current code, so
here are some fixes.

I have a few more patches, but those are slightly more complicated and
therefore not material for 0.1.

Cheers.

Felipe Contreras (9):
directconn: always check last flush status
io/http: fix read reset
io/http: check for AGAIN while reading content
timer: add stop()
io/http: stop timer after writing
io/http: improve http headers
io/http: reorganize some code
io/http: make sure data is written
io/http: add support for delayed flush

cvr/pn_direct_conn.c | 3 +-
io/pn_http_server.c | 96 +++++++++++++++++++++++++++++++++++--------------
pn_timer.h | 10 +++++-
3 files changed, 79 insertions(+), 30 deletions(-)

Felipe Contreras

unread,
Feb 10, 2010, 3:03:29 PM2/10/10
to msn-...@googlegroups.com, Felipe Contreras
Perhaps there will be nothing else to read the next time?

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---
cvr/pn_direct_conn.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/cvr/pn_direct_conn.c b/cvr/pn_direct_conn.c
index 1e9e46a..cb83d19 100644
--- a/cvr/pn_direct_conn.c
+++ b/cvr/pn_direct_conn.c
@@ -71,7 +71,8 @@ write_cb (GIOChannel *source,

if (direct_conn->last_flush == G_IO_STATUS_AGAIN) {
direct_conn->last_flush = pn_stream_flush(direct_conn->conn->stream, NULL);
- return TRUE;
+ if (direct_conn->last_flush == G_IO_STATUS_AGAIN)
+ return TRUE;
}

direct_conn->write_watch = 0;
--
1.6.6.1

Felipe Contreras

unread,
Feb 10, 2010, 3:03:30 PM2/10/10
to msn-...@googlegroups.com, Felipe Contreras
It turns out we need to wait until we finish reading the whole http
message.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

io/pn_http_server.c | 15 +++++----------
1 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/io/pn_http_server.c b/io/pn_http_server.c
index d7d8d4d..0e3c0bd 100644
--- a/io/pn_http_server.c
+++ b/io/pn_http_server.c
@@ -168,15 +168,6 @@ read_cb (GIOChannel *source,
}
}

- if (conn->status == PN_NODE_STATUS_OPEN)
- {
- http_conn->waiting_response = FALSE;
-
- pn_timer_restart (http_conn->timer);
-
- process_queue (http_conn, &conn->error);
- }
-
if (conn->error)
{
pn_node_error (conn);
@@ -805,8 +796,12 @@ read_impl (PnNode *conn,

pn_log ("con_len=%d,read=%zu", http_conn->content_length, bytes_read);

- if (http_conn->content_length == 0)
+ if (http_conn->content_length == 0) {
http_conn->parser_state = 0;
+ http_conn->waiting_response = FALSE;
+ pn_timer_restart (http_conn->timer);
+ process_queue (http_conn, &conn->error);
+ }
}

leave:
--
1.6.6.1

Felipe Contreras

unread,
Feb 10, 2010, 3:03:31 PM2/10/10
to msn-...@googlegroups.com, Felipe Contreras
Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---
io/pn_http_server.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/io/pn_http_server.c b/io/pn_http_server.c
index 0e3c0bd..17c8218 100644
--- a/io/pn_http_server.c
+++ b/io/pn_http_server.c
@@ -786,6 +786,9 @@ read_impl (PnNode *conn,

status = pn_stream_read (conn->stream, buf, MIN (http_conn->content_length, count), &bytes_read, &tmp_error);

+ if (status == G_IO_STATUS_AGAIN)
+ return status;
+
pn_log ("status=%d", status);
pn_log ("bytes_read=%zu", bytes_read);

--
1.6.6.1

Felipe Contreras

unread,
Feb 10, 2010, 3:03:32 PM2/10/10
to msn-...@googlegroups.com, Felipe Contreras
Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---
pn_timer.h | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/pn_timer.h b/pn_timer.h
index 39498c3..56899cd 100644
--- a/pn_timer.h
+++ b/pn_timer.h
@@ -65,7 +65,8 @@ pn_timer_start(struct pn_timer *timer,
static inline void
pn_timer_restart(struct pn_timer *timer)
{
- g_source_remove(timer->id);
+ if (timer->id)
+ g_source_remove(timer->id);
timer->id = g_timeout_add_seconds(timer->interval,
timer->function,
timer->data);
@@ -77,4 +78,11 @@ pn_timer_cancel(struct pn_timer *timer)
timer->id = 0;
}

+static inline void
+pn_timer_stop(struct pn_timer *timer)
+{
+ g_source_remove(timer->id);
+ timer->id = 0;
+}
+
#endif /* PN_TIMER_H */
--
1.6.6.1

Felipe Contreras

unread,
Feb 10, 2010, 3:03:33 PM2/10/10
to msn-...@googlegroups.com, Felipe Contreras
This way poll() is not called unnecessarily.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

io/pn_http_server.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/io/pn_http_server.c b/io/pn_http_server.c
index 17c8218..b8d0696 100644
--- a/io/pn_http_server.c
+++ b/io/pn_http_server.c
@@ -322,6 +322,7 @@ http_poll (gpointer data)
{
pn_log ("bytes_written=%zu", bytes_written);
http_conn->waiting_response = TRUE;
+ pn_timer_stop (http_conn->timer);
}
}

@@ -929,6 +930,7 @@ foo_write (PnNode *conn,
{
pn_log ("bytes_written=%zu", bytes_written);
http_conn->waiting_response = TRUE;
+ pn_timer_stop (http_conn->timer);
if (http_conn->cur)
g_object_unref (http_conn->cur);
http_conn->cur = prev;
--
1.6.6.1

Felipe Contreras

unread,
Feb 10, 2010, 3:03:34 PM2/10/10
to msn-...@googlegroups.com, Felipe Contreras
aMSN does something similar so probably it's ok :)

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

io/pn_http_server.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/io/pn_http_server.c b/io/pn_http_server.c
index b8d0696..3f3683f 100644
--- a/io/pn_http_server.c
+++ b/io/pn_http_server.c
@@ -290,13 +290,13 @@ http_poll (gpointer data)

header = g_strdup_printf ("POST http://%s/gateway/gateway.dll?%s HTTP/1.1\r\n"
"Accept: */*\r\n"
- "Accept-Language: en-us\r\n"
"User-Agent: MSMSGS\r\n"
"Host: %s\r\n"
- "Proxy-Connection: Keep-Alive\r\n"
"%s" /* Proxy auth */
+ "Proxy-Connection: Keep-Alive\r\n"
"Connection: Keep-Alive\r\n"
"Pragma: no-cache\r\n"
+ "Cache-Control: no-cache\r\n"
"Content-Type: application/x-msn-messenger\r\n"
"Content-Length: 0\r\n\r\n",
http_conn->gateway,
@@ -867,13 +867,13 @@ foo_write (PnNode *conn,
/** @todo investigate why this returns NULL sometimes. */
header = g_strdup_printf ("POST http://%s/gateway/gateway.dll?%s HTTP/1.1\r\n"
"Accept: */*\r\n"
- "Accept-Language: en-us\r\n"
"User-Agent: MSMSGS\r\n"
"Host: %s\r\n"
- "Proxy-Connection: Keep-Alive\r\n"
"%s" /* Proxy auth */
+ "Proxy-Connection: Keep-Alive\r\n"
"Connection: Keep-Alive\r\n"
"Pragma: no-cache\r\n"
+ "Cache-Control: no-cache\r\n"
"Content-Type: application/x-msn-messenger\r\n"
"Content-Length: %zu\r\n\r\n",
http_conn->gateway,
--
1.6.6.1

Felipe Contreras

unread,
Feb 10, 2010, 3:03:35 PM2/10/10
to msn-...@googlegroups.com, Felipe Contreras
More logical. Also probably will fix a memory leak.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

io/pn_http_server.c | 29 +++++++++++++----------------
1 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/io/pn_http_server.c b/io/pn_http_server.c
index 3f3683f..689bcec 100644
--- a/io/pn_http_server.c
+++ b/io/pn_http_server.c
@@ -312,18 +312,17 @@ http_poll (gpointer data)

status = pn_stream_write_full (conn->stream, header, strlen (header), &bytes_written, &tmp_error);

+ g_free (header);
+
+ http_conn->waiting_response = TRUE;
+ pn_timer_stop (http_conn->timer);
+
if (status == G_IO_STATUS_NORMAL)
{
status = pn_stream_flush (conn->stream, &tmp_error);

- g_free (header);
-
if (status == G_IO_STATUS_NORMAL)
- {


pn_log ("bytes_written=%zu", bytes_written);

- http_conn->waiting_response = TRUE;
- pn_timer_stop (http_conn->timer);
- }
}

if (status != G_IO_STATUS_NORMAL)
@@ -923,23 +922,21 @@ foo_write (PnNode *conn,
}
#endif

+ http_conn->waiting_response = TRUE;
+ pn_timer_stop (http_conn->timer);
+
+ if (http_conn->cur)
+ g_object_unref (http_conn->cur);
+ http_conn->cur = prev;
+ g_object_ref (G_OBJECT (http_conn->cur));
+
if (status == G_IO_STATUS_NORMAL)
status = pn_stream_flush (conn->stream, &tmp_error);

if (status == G_IO_STATUS_NORMAL)
- {


pn_log ("bytes_written=%zu", bytes_written);

- http_conn->waiting_response = TRUE;
- pn_timer_stop (http_conn->timer);
- if (http_conn->cur)
- g_object_unref (http_conn->cur);
- http_conn->cur = prev;
- g_object_ref (G_OBJECT (http_conn->cur));
- }
else
- {
pn_error ("not normal");
- }

if (ret_bytes_written)
*ret_bytes_written = bytes_written;
--
1.6.6.1

Felipe Contreras

unread,
Feb 10, 2010, 3:03:36 PM2/10/10
to msn-...@googlegroups.com, Felipe Contreras
It would be more efficient to wait until the socket is ready.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

io/pn_http_server.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/io/pn_http_server.c b/io/pn_http_server.c
index 689bcec..808145d 100644
--- a/io/pn_http_server.c
+++ b/io/pn_http_server.c
@@ -319,7 +319,10 @@ http_poll (gpointer data)

if (status == G_IO_STATUS_NORMAL)
{
- status = pn_stream_flush (conn->stream, &tmp_error);
+ do {
+ pn_log ("flush");
+ status = pn_stream_flush (conn->stream, &tmp_error);
+ } while (status == G_IO_STATUS_AGAIN);

if (status == G_IO_STATUS_NORMAL)


pn_log ("bytes_written=%zu", bytes_written);

@@ -930,8 +933,12 @@ foo_write (PnNode *conn,
http_conn->cur = prev;
g_object_ref (G_OBJECT (http_conn->cur));

- if (status == G_IO_STATUS_NORMAL)
- status = pn_stream_flush (conn->stream, &tmp_error);


+ if (status == G_IO_STATUS_NORMAL) {

+ do {
+ pn_log ("flush");
+ status = pn_stream_flush (conn->stream, &tmp_error);
+ } while (status == G_IO_STATUS_AGAIN);


+ }

if (status == G_IO_STATUS_NORMAL)

pn_log ("bytes_written=%zu", bytes_written);

--
1.6.6.1

Felipe Contreras

unread,
Feb 10, 2010, 3:03:37 PM2/10/10
to msn-...@googlegroups.com, Felipe Contreras
Only try to flush when the socket is ready.

Signed-off-by: Felipe Contreras <felipe.c...@gmail.com>
---

io/pn_http_server.c | 52 +++++++++++++++++++++++++++++++++++++++++++-------
1 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/io/pn_http_server.c b/io/pn_http_server.c
index 808145d..2353433 100644
--- a/io/pn_http_server.c
+++ b/io/pn_http_server.c
@@ -59,6 +59,9 @@ struct PnHttpServer
GHashTable *childs;
PnNode *cur;
gchar *old_buffer;
+
+ guint write_watch;
+ GIOStatus last_flush;
};

struct PnHttpServerClass
@@ -245,6 +248,24 @@ get_auth(PnNode *conn)
#endif /* HAVE_LIBPURPLE */

static gboolean
+write_cb (GIOChannel *source,
+ GIOCondition condition,
+ gpointer data)
+{
+ PnHttpServer *http_conn = data;
+
+ if (http_conn->last_flush == G_IO_STATUS_AGAIN) {
+ http_conn->last_flush = pn_stream_flush(PN_NODE(http_conn)->stream, NULL);
+ if (http_conn->last_flush == G_IO_STATUS_AGAIN)
+ return TRUE;
+ }
+
+ http_conn->write_watch = 0;
+
+ return FALSE;
+}
+
+static gboolean
http_poll (gpointer data)
{
PnNode *conn;
@@ -319,10 +340,15 @@ http_poll (gpointer data)

if (status == G_IO_STATUS_NORMAL)
{
- do {
- pn_log ("flush");


- status = pn_stream_flush (conn->stream, &tmp_error);

- } while (status == G_IO_STATUS_AGAIN);
+ status = pn_stream_flush (conn->stream, &tmp_error);
+


+ if (status == G_IO_STATUS_AGAIN) {

+ http_conn->last_flush = status;
+ http_conn->write_watch = g_io_add_watch(conn->stream->channel,
+ G_IO_OUT, write_cb, http_conn);
+ /* fake status */
+ status = G_IO_STATUS_NORMAL;


+ }

if (status == G_IO_STATUS_NORMAL)
pn_log ("bytes_written=%zu", bytes_written);

@@ -513,6 +539,11 @@ close_impl (PnNode *conn)
pn_timer_free(http_conn->timer);
http_conn->timer = NULL;

+ if (http_conn->write_watch) {
+ g_source_remove (http_conn->write_watch);
+ http_conn->write_watch = 0;
+ }
+
g_free (http_conn->last_session_id);
http_conn->last_session_id = NULL;

@@ -934,10 +965,15 @@ foo_write (PnNode *conn,
g_object_ref (G_OBJECT (http_conn->cur));

if (status == G_IO_STATUS_NORMAL) {
- do {
- pn_log ("flush");


- status = pn_stream_flush (conn->stream, &tmp_error);

- } while (status == G_IO_STATUS_AGAIN);
+ status = pn_stream_flush (conn->stream, &tmp_error);
+


+ if (status == G_IO_STATUS_AGAIN) {

+ http_conn->last_flush = status;
+ http_conn->write_watch = g_io_add_watch(conn->stream->channel,
+ G_IO_OUT, write_cb, http_conn);
+ /* fake status */
+ status = G_IO_STATUS_NORMAL;


+ }
}

if (status == G_IO_STATUS_NORMAL)

--
1.6.6.1

Reply all
Reply to author
Forward
0 new messages