@abh3 requested changes on this pull request.
> std::string m_mapfile;
std::shared_ptr<const std::vector<MapfileEntry>> m_entries;
XrdHttpSecXtractor *m_xrdvoms{nullptr};
XrdSysError *m_edest{nullptr};
- std::atomic<time_t> m_last_update;
+ // Pipes to allow the main thread to communicate shutdown events to the maintenance
+ // thread, allowing for a clean shutdown.
+ int m_maintenance_pipe_r{-1};
+ int m_maintenance_pipe_w{-1};
+ int m_maintenance_thread_pipe_r{-1};
+ int m_maintenance_thread_pipe_w{-1};
+
+ // After success, how long to wait until the next mapfile check.
+ static constexpr unsigned m_update_interval = 30;
OK, so I gather that 30 seconds is not in the overkill territory. Frankly, I think it is and 60 or even 180 would be more appropriate.
> std::string m_mapfile;
std::shared_ptr<const std::vector<MapfileEntry>> m_entries;
XrdHttpSecXtractor *m_xrdvoms{nullptr};
XrdSysError *m_edest{nullptr};
- std::atomic<time_t> m_last_update;
+ // Pipes to allow the main thread to communicate shutdown events to the maintenance
+ // thread, allowing for a clean shutdown.
+ int m_maintenance_pipe_r{-1};
+ int m_maintenance_pipe_w{-1};
+ int m_maintenance_thread_pipe_r{-1};
+ int m_maintenance_thread_pipe_w{-1};
+
+ // After success, how long to wait until the next mapfile check.
+ static constexpr unsigned m_update_interval = 30;
+ // After failure, how long to wait until the next mapfile check.
+ static constexpr unsigned m_update_interval_failure = 3;
Nice addition but where is the limit? So, if someone makes a mistake we will start checking this thing every three seconds? Now, that is rather bad because there is no feedback other than the log and we will be doing a rather frequent check that is likely to fail all the time because the admin went to sleep. I am very skeptical about this check. In fact, let's say the file disappears, so now we will get a message in the log every three seconds filling up the log until someone looks at it. I think we should just use the default refresh time. This is rather dangerous.
> + return mapper->IsValid() ? mapper.get() : nullptr;
+}
+
+
+void *
+XrdVomsMapfile::MaintenanceThread(void *myself_raw)
+{
+ auto myself = static_cast<XrdVomsMapfile*>(myself_raw);
+
+ auto now = monotonic_time_s();
+ auto next_update = now + m_update_interval;
+ while (true) {
+ now = monotonic_time_s();
+ auto remaining = next_update - now;
+ struct pollfd fds;
+ fds.fd = myself->m_maintenance_pipe_r;
My take is this is only for the case when the object is destroyed and you want to stop the maintenance thread. OK, I get that but the object will never be destroyed as far as I can see. Once the mapfile is started it continues for the life of the server. So, I am confused as to what problem this is trying to solve.
--
Reply to this email directly or view it on GitHub:
https://github.com/xrootd/xrootd/pull/1572#pullrequestreview-861854279
You are receiving this because you are subscribed to this thread.
Message ID: <[log in to unmask]>
########################################################################
Use REPLY-ALL to reply to list
To unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1
