Append history in interactive sessions #99

ylluminarious · 2018-07-04T00:48:31Z

As per the discussion here. Please let me know if you have any improvements in mind. Pinging @raimue since we spoke about this together on the aforementioned Trac ticket.

raimue

I am sorry I did not review your patch on Trac. My time for MacPorts is currently a bit scarce, but the pull request makes it easier. Thank you for that.

The changes look good to me, although I did not test them. I would appreciate if you could use the usual trailer with "Closes: ..." as reference to the ticket.

raimue · 2018-07-04T13:14:47Z

src/pextlib1.0/readline.c

+
+		hist_file = fopen(s, "a");
+		fprintf(hist_file, "%s\n", current_history()->line);
+		fclose(hist_file);


There is a chance for a race condition here when multiple port instances are running at the same time. Although it is very unlikely that anyone would be able to type in both port instances at the same time, so we could just ignore it.

@raimue Hmm, okay, I will see if I can address this with lockfiles or the like. If things get too hairy, I may just ignore it as you say.

ylluminarious · 2018-07-04T18:38:53Z

@raimue

I am sorry I did not review your patch on Trac. My time for MacPorts is currently a bit scarce, but the pull request makes it easier. Thank you for that.

No problem, I figured you were probably too busy to respond on Trac. I definitely understand not having time for things like Macports sometimes; real life can be a pain! But you're welcome, and thank you for the kindness.

The changes look good to me, although I did not test them. I would appreciate if you could use the usual trailer with "Closes: ..." as reference to the ticket.

Okay, I will try to do that in the future. Where am I supposed to put the "Closes: ..." remark? In the title of the PR?

jmroot · 2018-07-04T23:17:45Z

Okay, I will try to do that in the future. Where am I supposed to put the "Closes: ..." remark? In the title of the PR?

In the commit message body, on a line by itself.
https://trac.macports.org/wiki/CommitMessages

ylluminarious · 2018-07-05T03:35:35Z

@jmroot Oh, okay. I had read that article previously, but I missed the part which gives an example of that. Thanks for pointing me to it!

The only interesting thing here is that apparently we don't need the special encoding that is currently used in the history file. More details here: https://trac.macports.org/ticket/56381#comment:10

Hitherto, port.tcl has used bash-style history-writing at the end of an interactive session. That is vulnerable to crashes and other abrupt interruptions. This changes that.

As discussed here: macports#99 (comment) The first hunk which changes _XOPEN_SOURCE to _BSD_SOURCE is due to the fact that flock(2) doesn't work when _XOPEN_SOURCE is defined as such. According to an answer on StackOverflow, using _BSD_SOURCE is a valid alternative for strdup(3). See https://stackoverflow.com/questions/5378778/what-does-d-xopen-source-do-mean#5724485

ylluminarious · 2018-07-31T06:43:17Z

@raimue I have added some file-locking code as discussed above. Sorry for the delay; I've been quite busy but finally managed to make some time for this. Please let me know if anything else should be done. Also, should I squash my commits into a single one to have the "Closes: ..." trailer?

pmetzger · 2018-07-31T12:43:48Z

You must indeed always squash your commits. (I have no opinion on the PR itself...)

ylluminarious · 2018-07-31T17:38:55Z

@pmetzger Ok, thanks for the info.

ylluminarious · 2018-08-18T01:38:04Z

@raimue Friendly ping... any thoughts on this? :)

neverpanic

Needs an fflush(3) to work correctly, and while at it please sprinkle some error handling in, then it's good to go.

neverpanic · 2018-08-20T21:50:53Z

src/pextlib1.0/readline.c

 		hist_file = fopen(s, "a");
+
+		flock(fileno(hist_file), LOCK_EX);
 		fprintf(hist_file, "%s\n", current_history()->line);


Unless hist_file is a terminal or stderr, this fprintf(3) call will be buffered, which means it will not actually trigger writing to disk, rendering your locking moot against modification across multiple instances of macports writing at the same time.

You must either switch hist_file to line buffering mode using setvbuf(3), or call fflush(3) after writing the line and before returning the lock. I'd probably just call fflush(3), it's simpler.

@neverpanic Ok, I went ahead and took your advice for fflush. Strangely, what you're saying makes perfect sense, but when I tested this out, the locking mechanism actually worked. It was as though it didn't need a flush. I tested it by opening two separate terminals and entering commands into them via CloneKeys. I'm curious now; do you know why that may have happened?

neverpanic · 2018-08-20T22:02:19Z

src/pextlib1.0/readline.c

+		}
+		s = Tcl_GetString(objv[2]);
+
+		hist_file = fopen(s, "a");


Can you please add error handling to the fopen(3) call here? mkdtemp.c has an example on how to do this, e.g.

if (NULL == (hist_file = fopen(s, "a"))) { Tcl_AppendResult(interp, "fopen(", s, "): ", strerror(errno), NULL); return TCL_ERROR; }

neverpanic · 2018-08-20T22:06:46Z

src/pextlib1.0/readline.c

+			return TCL_ERROR;
+		}
+		s = Tcl_GetString(objv[2]);
+		hist_file = fopen(s, "a");


Thank you GitHub review for hiding my review comment against the version in one of the lower commits. Please add error handling here. See mkdtemp.c for good examples. E.g., use:

if (NULL == (hist_file = fopen(s, "a"))) { Tcl_AppendResult(interp, "fopen(", s, "): ", strerror(errno), NULL); return TCL_ERROR; }

@neverpanic Thanks for the code, I went ahead and pasted it in verbatim :)

I also added some error-checking for the fprintf call; please let me know if it's okay. It's not as pretty as the code in mktemp.c, but it should do the trick.

ylluminarious · 2018-08-21T00:48:35Z

@neverpanic I went ahead and implemented your changes as you requested. Please let me know if they're satisfactory. I also added some comments for visual and conceptual clarity since I stared at this code without comments and it felt like my eyes were getting poked with curly-braced needles :)

neverpanic · 2018-08-21T14:04:08Z

Thanks!

ylluminarious · 2018-08-21T14:07:04Z

@neverpanic You're welcome! Thanks for merging it and for the help!

Did you see my comment from last night wherein I had a question about the flushing seeming unnecessary? I'm curious to know what may have been going on... :)

neverpanic · 2018-08-23T18:24:51Z

I think what you were seeing is that the fclose(3) at the end of the process will flush the buffers anyway, i.e. it will internally call fflush(3). However, it won't do that under the lock.

It may just be very hard to hit the race condition that would make the problem visible (and even if two writes were happening at the same time outside of the lock, they may still end up being a single write(2) syscall and not interfere with each other). To demonstrate the race condition in reality, you'd probably have to write huge commands (>= pagesize) at the exact same time in two terminals.

ylluminarious · 2018-08-24T00:41:51Z

@neverpanic Thanks so much for the explanation! This makes more sense to me now. I suspected that fclose was behind this. I guess the lock gave just enough of a delay that the flushing done by fclose happens right before the acquisition of the lock for the competing process, or at least before that competing process was able to do its flush. Thus, there was a de facto order of operations, although unreliable.

Yes, such a situation would indeed make it hard to expose the problem. That is a very hard-to-hit race condition, although possible. And as you also said, if the two writes were mixed into the same syscall then there would be no conflict anyway. Maybe I'll try that test you mention someday, just for fun :D

raimue reviewed Jul 4, 2018

View reviewed changes

ylluminarious added 3 commits July 29, 2018 14:55

ad-hoc support for history appension in readline.c

af2253a

The only interesting thing here is that apparently we don't need the special encoding that is currently used in the history file. More details here: https://trac.macports.org/ticket/56381#comment:10

make port.tcl use history appension

0a4a2f4

Hitherto, port.tcl has used bash-style history-writing at the end of an interactive session. That is vulnerable to crashes and other abrupt interruptions. This changes that.

ylluminarious force-pushed the append_history branch from f5683ea to bc55bfd Compare July 31, 2018 06:37

pmetzger requested a review from neverpanic August 18, 2018 10:55

neverpanic requested changes Aug 20, 2018

View reviewed changes

neverpanic reviewed Aug 20, 2018

View reviewed changes

added some improvements suggested by @neverpanic

09a912b

oops, forgot to include errno.h

09dac34

neverpanic approved these changes Aug 21, 2018

View reviewed changes

neverpanic merged commit 486565d into macports:master Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Append history in interactive sessions #99

Append history in interactive sessions #99

ylluminarious commented Jul 4, 2018

raimue left a comment

raimue Jul 4, 2018

ylluminarious Jul 4, 2018

ylluminarious commented Jul 4, 2018

jmroot commented Jul 4, 2018

ylluminarious commented Jul 5, 2018

ylluminarious commented Jul 31, 2018

pmetzger commented Jul 31, 2018

ylluminarious commented Jul 31, 2018

ylluminarious commented Aug 18, 2018

neverpanic left a comment

neverpanic Aug 20, 2018

ylluminarious Aug 21, 2018

neverpanic Aug 20, 2018

neverpanic Aug 20, 2018

ylluminarious Aug 21, 2018

ylluminarious commented Aug 21, 2018

neverpanic commented Aug 21, 2018

ylluminarious commented Aug 21, 2018

neverpanic commented Aug 23, 2018

ylluminarious commented Aug 24, 2018

Append history in interactive sessions #99

Append history in interactive sessions #99

Conversation

ylluminarious commented Jul 4, 2018

raimue left a comment

Choose a reason for hiding this comment

raimue Jul 4, 2018

Choose a reason for hiding this comment

ylluminarious Jul 4, 2018

Choose a reason for hiding this comment

ylluminarious commented Jul 4, 2018

jmroot commented Jul 4, 2018

ylluminarious commented Jul 5, 2018

ylluminarious commented Jul 31, 2018

pmetzger commented Jul 31, 2018

ylluminarious commented Jul 31, 2018

ylluminarious commented Aug 18, 2018

neverpanic left a comment

Choose a reason for hiding this comment

neverpanic Aug 20, 2018

Choose a reason for hiding this comment

ylluminarious Aug 21, 2018

Choose a reason for hiding this comment

neverpanic Aug 20, 2018

Choose a reason for hiding this comment

neverpanic Aug 20, 2018

Choose a reason for hiding this comment

ylluminarious Aug 21, 2018

Choose a reason for hiding this comment

ylluminarious commented Aug 21, 2018

neverpanic commented Aug 21, 2018

ylluminarious commented Aug 21, 2018

neverpanic commented Aug 23, 2018

ylluminarious commented Aug 24, 2018