Improved implementation of Memory.capture.

Author: samuel-williams-shopify

lib/process/metrics/memory/darwin.rb

@@ -51,18 +51,18 @@ def self.parse_size(string)
 
 			# Capture memory usage for the given process IDs.
 			def self.capture(pid, count: 1, **options)
-				usage = Memory.zero
-				
 				IO.popen(["vmmap", pid.to_s], "r") do |io|
+					usage = Memory.zero
+					
 					io.each_line do |line|
 						if match = LINE.match(line)
+							usage.map_count += 1
+							
 							virtual_size = parse_size(match[:virtual_size])
 							resident_size = parse_size(match[:resident_size])
 							dirty_size = parse_size(match[:dirty_size])
 							swap_size = parse_size(match[:swap_size])
 
-							# Update counts
-							usage.map_count += 1
 							usage.resident_size += resident_size
 							usage.swap_size += swap_size
 
@@ -84,13 +84,21 @@ def self.capture(pid, count: 1, **options)
 							end
 						end
 					end
+					
+					if usage.map_count.zero?
+						# vmap might not fail, but also might not return any data.
+						return nil
+					end
+					
+					# Darwin does not expose proportional memory usage, so we guess based on the number of processes. Yes, this is a terrible hack, but it's the most reasonable thing to do given the constraints:
+					usage.proportional_size = usage.resident_size / count
+					usage.proportional_swap_size = usage.swap_size / count
+					
+					return usage
 				end
-				
-				# Darwin does not expose proportional memory usage, so we guess based on the number of processes. Yes, this is a terrible hack, but it's the most reasonable thing to do given the constraints:
-				usage.proportional_size = usage.resident_size / count
-				usage.proportional_swap_size = usage.swap_size / count
-				
-				return usage
+			rescue Errno::ESRCH
+				# Process doesn't exist.
+				return nil
 			end
 		end
 

lib/process/metrics/memory/linux.rb

@@ -20,7 +20,7 @@ def self.capture_faults(pid, usage)
 				usage.minor_faults = fields[10-3].to_i
 				usage.major_faults = fields[12-3].to_i
 			rescue
-				# Be robust to unexpected formats; ignore errors silently.
+				# Ignore.
 			end
 
 			# @returns [Numeric] Total memory size in kilobytes.
@@ -54,10 +54,10 @@ def self.supported?
 
 				# Capture memory usage for the given process IDs.
 				def self.capture(pid, **options)
-					usage = Memory.zero
-					
-					begin
-						File.foreach("/proc/#{pid}/smaps_rollup") do |line|
+					File.open("/proc/#{pid}/smaps_rollup") do |file|
+						usage = Memory.zero
+						
+						file.each_line do |line|
 							if /(?<name>.*?):\s+(?<value>\d+) kB/ =~ line
 								if key = SMAP[name]
 									usage[key] += value.to_i
@@ -68,11 +68,12 @@ def self.capture(pid, **options)
 						usage.map_count += File.readlines("/proc/#{pid}/maps").size
 						# Also capture fault counters:
 						self.capture_faults(pid, usage)
-					rescue Errno::ENOENT, Errno::ESRCH
-						# Ignore, process may have ended.
+						
+						return usage
 					end
-					
-					return usage
+				rescue Errno::ENOENT, Errno::ESRCH
+					# Process doesn't exist.
+					return nil
 				end
 			elsif File.readable?("/proc/self/smaps")
 				# Whether the memory usage can be captured on this system.
@@ -82,10 +83,10 @@ def self.supported?
 
 				# Capture memory usage for the given process IDs.
 				def self.capture(pid, **options)
-					usage = Memory.zero
-					
-					begin
-						File.foreach("/proc/#{pid}/smaps") do |line|
+					File.open("/proc/#{pid}/smaps") do |file|
+						usage = Memory.zero
+						
+						file.each_line do |line|
 							# The format of this is fixed according to:
 							# https://github.com/torvalds/linux/blob/351c8a09b00b5c51c8f58b016fffe51f87e2d820/fs/proc/task_mmu.c#L804-L814
 							if /(?<name>.*?):\s+(?<value>\d+) kB/ =~ line
@@ -98,13 +99,15 @@ def self.capture(pid, **options)
 								usage.map_count += 1
 							end
 						end
+						
 						# Also capture fault counters:
 						self.capture_faults(pid, usage)
-					rescue Errno::ENOENT, Errno::ESRCH
-						# Ignore, process may have ended.
+						
+						return usage
 					end
-					
-					return usage
+				rescue Errno::ENOENT, Errno::ESRCH
+					# Process doesn't exist.
+					return nil
 				end
 			else
 				def self.supported?

releases.md

@@ -1,5 +1,9 @@
 # Releases
 
+## Unreleased
+
+  - Be more proactive about returning nil if memory capture failed.
+
 ## v0.6.1
 
   - Handle `Errno::ESRCH: No such process @ io_fillbuf - fd:xxx /proc/xxx/smaps_rollup` by ignoring it.