Skip to content

Teach CommonLogger how to deal with hijack'd requests #587

Closed
wants to merge 1 commit into from

3 participants

@evanphx
evanphx commented Jul 18, 2013

With hijack requests, the output from CommonLogger is, well, weird. This at least lets the logger make it more aware that hijacking took place and that this log entry doesn't represent data actually being sent to the client.

@rkh rkh commented on the diff Jul 18, 2013
lib/rack/commonlogger.rb
@@ -32,7 +33,16 @@ def call(env)
began_at = Time.now
status, header, body = @app.call(env)
header = Utils::HeaderHash.new(header)
- body = BodyProxy.new(body) { log(env, status, header, began_at) }
+
+ # If we've been hijacked, then output a special line
+ if env['rack.hijack_io']
+ log_hijacking(env, 'HIJACK', header, began_at)
+ elsif ary = env['rack.after_reply']
@rkh
Official Rack repositories member
rkh added a note Jul 18, 2013

This is Puma specific code, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rkh
Official Rack repositories member
rkh commented Jul 18, 2013

This generally looks good, no tests though, and the rack.after_reply part is not in the spec atm, actually Puma is even somewhat violating the spec right now by using the rack. prefix for non-standard things. Maybe we should make this part of the spec rather?

@evanphx
evanphx commented Jul 22, 2013

I'm happy to add tests, etc. I wanted mostly to propose this, which means adding rack.after_reply to the spec probably.

@rkh
Official Rack repositories member
rkh commented Jul 22, 2013

I'm generally +1 on this.

@raggi
Official Rack repositories member
raggi commented Dec 4, 2013

I would love to get a quick summary of the semantics and typing of rack.after_reply, and get that added to the spec. I'm then ready to merge.

@krasnoukhov krasnoukhov referenced this pull request in tmm1/gctools Jan 30, 2014
Merged

Puma middleware #1

@raggi raggi added this to the Rack 1.6 milestone Jul 12, 2014
@raggi
Official Rack repositories member
raggi commented Jul 12, 2014

Will accept for 1.6 if Lint & SPEC are adjusted appropriately and only introduce new features with opt-in semantics.

@raggi
Official Rack repositories member
raggi commented Aug 3, 2014

Closing waiting for response to review feedback.

@raggi raggi closed this Aug 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.