search

ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
891 stars 232 forks source link

The option cookie_ignore_informational doesn't work in GUN. #226

Closed rody-liu closed 4 years ago

rody-liu commented 4 years ago

When cookie_ignore_informational is included in the GUN options, the GUN instance can't be created since the option is missed in gun:check_options(). When the option is set to true, the set_cookie commands should not be queued in the gun_http/gun_http2 modules.

When the option is set as false to enable the cookie related functions, there is a potential performance risk. The queued commands set_cookie are not removed from the queue. And in the gun_http2:handle_ret() function, the queue list is doing reverse to add each command. It's better use erlang queue data structure which has generally O(1) complexity in/out operation and converting to list.

essen commented 4 years ago

Thanks for the check_options I'll fix it.

I don't really see much point in filtering early on for the informational option, I doubt you'd see a difference. It can always be improved later on if there is one.

The commands_queue must be cleared, good catch. It's not a performance risk, it's just a bug, since cookies will be set multiple times.

I doubt that the lists:reverse is worse than queue. We're talking about 2 or 3 commands here, plus the cookies if any. Converting everything to queue is unlikely to improve anything.

essen commented 4 years ago

Fixed the two issues. Thanks!

rody-liu commented 4 years ago

Thanks for the fixing. When the cookie_ignore_informational is set to true, the set_cookie comments will be still queued in the gun_http/gun_http2. And the gun module do nothing for the commands when the option is true. I suspect it will be a memory leak here?

essen commented 4 years ago

That's not a memory leak, why would it be? We return them and then discard them almost immediately.

rody-liu commented 4 years ago

The set_cookie commands is queued in the list #http2_state.commands_queue. But no places remove them from the list. I just verified that could happen. After return the commands to gun module, the queue list should be empty and should be updated into the loop data of http2_state.

essen commented 4 years ago

As I've said above I've fixed this issue, it's in a3308b8e4771eff64011bc683c7bdd94b79ceff3.

rody-liu commented 4 years ago

Thank you for your answers. Cheers!